[HOLD for payment 2023-03-13] [$4000] LHN - Workspace badge is not fully visible/missing for some Room chats #14491
Comments
kbecciv
added
Daily
|
Triggered auto assignment to @JmillsExpensify ( |
|
Wow this one is pretty wild. I need another day to circle back and close the loop on this one. |
|
Alright, sorry for the delay! I'm able to reproduce this one. |
|
That said, @shawnborton it'd be good to get your take on this one. Like on the one hand, I'm not sure if we should optimize for showing the workspace badge in this case, because the the room name is ultimately more important. Overall, I'm not convinced this is a bug. I'm going to hold off triaging it until we get another set of eyes. |
|
I think this is such an edge case. That being said, one easy way to fix this would be to give the badge a min-width so that in the very least, you can always see at least 3-4 characters of the badge as well. I think ideally the room name and badge would be truncated via ellipsis, I could have sworn that's how it was implemented. @bernhardoj do you have any thoughts on this? Since you worked on this PR here. |
|
@bernhardoj can't comment on this issue, but sent me this on Slack:
So I say we open this up as a bug and allow @bernhardoj to submit a proposal to fix since he originally worked on this. I don't think this is a regression because it's working as expected from the original PR, but this was an oversight in our testing. |
JmillsExpensify
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~016ac522930fd1c9d2 |
|
Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
melvin-bot
bot
added
the
Help Wanted
|
Cool, agreed. Pretty significant edge case. Opening up the issue for now. |
|
Triggered auto assignment to @grgia ( |
|
I think this is such an edge case, too. That being said, one easy way to fix this would be to give the badge a min-width so that in the very least, you can always see at least 3-4 characters of the badge as well. And there should be one thing to do. I can totally settle this issue in a day if assigned. |
ProposalThe room name isn't shrinking because we're using Line 1198 in 703b5c4 We can fix this either by removing flex-shrink: 0 from above OR we can pass styles.flexShrink1 below (This can be done conditionally since we're using TextPill component only when optionItem.isChatRoom is true)
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ProposalI can't find a "pure css" way to solve this issue, so I need to use the help of We will set the max width of the display name to 75% of the total width IF the text pill width >= 25% of the total width. Else, the max width will be (total width - text pill width). I move the whole logic to a new component called import React from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../styles/styles';
import * as StyleUtils from '../../styles/StyleUtils';
import DisplayNames from '../DisplayNames';
import TextPill from '../TextPill';
const propTypes = {
/** Option row data */
optionItem: PropTypes.object.isRequired,
displayNameStyle: PropTypes.arrayOf(PropTypes.object),
textPillStyle: PropTypes.arrayOf(PropTypes.object),
}
const defaultProps = {
displayNameStyle: [],
textPillStyle: [],
}
class OptionRowTitle extends React.Component {
constructor(props) {
super(props);
this.state = {
width: 0,
textPillWidth: 0,
}
}
render() {
let displayNameMaxWidth = '100%';
if (this.props.optionItem.isChatRoom) {
displayNameMaxWidth = this.state.width > 0 && this.state.textPillWidth > 0
? Math.max(this.state.width * 75/100, this.state.width - this.state.textPillWidth)
: '75%';
}
return (
<View
onLayout={e => this.setState({width: e.nativeEvent.layout.width})}
style={[styles.flexRow, styles.alignItemsCenter, styles.mw100, styles.overflowHidden]}
>
<DisplayNames
accessibilityLabel="Chat user display names"
fullTitle={this.props.optionItem.text}
displayNamesWithTooltips={this.props.optionItem.displayNamesWithTooltips}
tooltipEnabled
numberOfLines={1}
textStyles={[
...this.props.displayNameStyle,
StyleUtils.getMaxWidthStyle(displayNameMaxWidth),
]}
shouldUseFullTitle={this.props.optionItem.isChatRoom || this.props.optionItem.isPolicyExpenseChat}
/>
{this.props.optionItem.isChatRoom && (
<TextPill
style={this.props.textPillStyle}
accessibilityLabel="Workspace name"
text={this.props.optionItem.subtitle}
onLayout={e => this.setState({textPillWidth: e.nativeEvent.layout.width + 4})}
/>
)}
</View>
)
}
}
OptionRowTitle.propTypes = propTypes;
OptionRowTitle.defaultProps = defaultProps;
export default OptionRowTitle;The initial max width value is 100% in case the text pill will not be shown. We wait for the diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js
index e64ed774b8..be998c6454 100644
--- a/src/components/LHNOptionsList/OptionRowLHN.js
+++ b/src/components/LHNOptionsList/OptionRowLHN.js
@@ -12,7 +12,6 @@ import Icon from '../Icon';
import * as Expensicons from '../Icon/Expensicons';
import MultipleAvatars from '../MultipleAvatars';
import Hoverable from '../Hoverable';
-import DisplayNames from '../DisplayNames';
import IOUBadge from '../IOUBadge';
import colors from '../../styles/colors';
import withLocalize, {withLocalizePropTypes} from '../withLocalize';
@@ -22,8 +21,8 @@ import CONST from '../../CONST';
import variables from '../../styles/variables';
import themeColors from '../../styles/themes/default';
import SidebarUtils from '../../libs/SidebarUtils';
-import TextPill from '../TextPill';
import OfflineWithFeedback from '../OfflineWithFeedback';
+import OptionRowTitle from './OptionRowTitle';
const propTypes = {
/** Style for hovered state */
@@ -67,7 +66,7 @@ const OptionRowLHN = (props) => {
: styles.sidebarLinkText;
const textUnreadStyle = optionItem.isUnread
? [textStyle, styles.sidebarLinkTextBold] : [textStyle];
- const displayNameStyle = StyleUtils.combineStyles([styles.optionDisplayName, styles.optionDisplayNameCompact, ...textUnreadStyle], props.style);
+ const displayNameStyle = StyleUtils.combineStyles([styles.optionDisplayName, ...textUnreadStyle], props.style);
const textPillStyle = props.isFocused
? [styles.ml1, StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.icon, 0.5)]
: [styles.ml1];
@@ -164,24 +163,12 @@ const OptionRowLHN = (props) => {
)
}
<View style={contentContainerStyles}>
- <View style={[styles.flexRow, styles.alignItemsCenter, styles.mw100, styles.overflowHidden]}>
- <DisplayNames
- accessibilityLabel="Chat user display names"
- fullTitle={optionItem.text}
- displayNamesWithTooltips={optionItem.displayNamesWithTooltips}
- tooltipEnabled
- numberOfLines={1}
- textStyles={displayNameStyle}
- shouldUseFullTitle={optionItem.isChatRoom || optionItem.isPolicyExpenseChat}
- />
- {optionItem.isChatRoom && (
- <TextPill
- style={textPillStyle}
- accessibilityLabel="Workspace name"
- text={optionItem.subtitle}
- />
- )}
- </View>
+ <OptionRowTitle
+ optionItem={optionItem}
+ displayNameStyle={displayNameStyle}
+ textPillStyle={textPillStyle}
+ key={props.viewMode}
+ />
{optionItem.alternateText ? (
<Text
style={alternateTextStyle}
diff --git a/src/styles/StyleUtils.js b/src/styles/StyleUtils.js
index 40bf28b37b..aa642c50cc 100644
--- a/src/styles/StyleUtils.js
+++ b/src/styles/StyleUtils.js
@@ -182,6 +182,16 @@ function getWidthStyle(width) {
};
}
+/**
+ * @param {String|number} maxWidth
+ * @returns
+ */
+function getMaxWidthStyle(maxWidth) {
+ return {
+ maxWidth,
+ };
+}
+
/**
* Returns a style with backgroundColor and borderColor set to the same color
*
@@ -670,6 +680,7 @@ export {
getZoomCursorStyle,
getZoomSizingStyle,
getWidthStyle,
+ getMaxWidthStyle,
getBackgroundAndBorderStyle,
getBackgroundColorStyle,
getBackgroundColorWithOpacityStyle,
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 4b1d1982ae..eaae8bed7c 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -1188,14 +1188,8 @@ const styles = {
fontFamily: fontFamily.EXP_NEUE,
height: variables.alternateTextHeight,
lineHeight: variables.lineHeightXLarge,
- ...whiteSpace.noWrap,
- },
-
- optionDisplayNameCompact: {
- minWidth: 'auto',
- flexBasis: 'auto',
- flexGrow: 0,
flexShrink: 0,
+ ...whiteSpace.noWrap,
},
displayNameTooltipEllipsis: {
I remove the We can additionally set the I don't have access to Room so I change the display name and text pill state directly from the code, just for testing Result Compact Mode |
|
Ah interesting, looks like @daraksha-dk was able to find a "CSS-only" approach to this? |
|
If the flex shrink is removed or set to 1, this will happen when the display name text is short and the workspace name is long. Basically, the same issue with this #11799 (comment). That's why we decided to use the flex shrink 0. |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.78-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-03-13. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
|
For the BZ checklist, I don't think this is as much of a bug caused than an edge case we didn't account for. Perhaps we need to encourage thinking about edge cases in the design process for these features. i.e how will this look with long/short text, how will this shrink/grow, etc. What do you think @JmillsExpensify? |
|
Where are we at on payments for this issue? @JmillsExpensify |
|
I think everyone was busy at ECX last week. Hope BZ checklist and payment would be sorted this week. |
|
Timeline: |
|
Not overdue |
|
Sorry ya'll just coming back to this while all the travel and OOO. Ok so in light of the post above, this issue is also eligible for the urgency bonus. Payments would be as follows:
Upwork job is here: https://www.upwork.com/jobs/~016b936b9b835a55e8. I've invited both contributors to the job, and the bonus will be paid out when the payment is processed. That said, @situchan can you confirm where the suggested regression test is? It's checked off above but I'm not clear where you've posted it and we've agreed that it's appropriate. |
|
Sorry I thought it won't require regression step based on #14491 (comment). Regression Test Proposal
Do we agree 👍 or 👎 |
|
@JmillsExpensify upwork says "This offer is not available anymore" |
|
@aimane-chnaif ah Upwork oddities sorry about that. I just sent you a new contract for this Upwork job: https://www.upwork.com/jobs/~01583e015bbae3a6a4. In other news, @situchan has been compensated and the contract is closed via Upwork. Thank you for the testing steps! |
|
@JmillsExpensify accepted, thanks! |
|
All paid out! Regression testing steps are also linked above, so I think we're finally good to close this one out now. 🙌🏼 |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
WS badge is fully visible and present for all chats
Actual Result:
Workspace badge is not fully visible/missing for some Room chats
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.58.2
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: Applause - internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: