-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for Santhosh] [$1000] mWeb - New room - Room names in LHN do not match the name when created #20510
Comments
Triggered auto assignment to @NicMendonca ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.mWeb - New room - Room names in LHN do not match the name when created What is the root cause of that problem?Let take a look at this piece of code: App/src/components/DisplayNames/index.js Lines 108 to 114 in f64a388
As we can see we will render the tooltip when this.props.displayNamesWithTooltips.length > 1 and this.state.isEllipsisActive is true.App/src/components/DisplayNames/index.js Line 23 in f64a388
The problem is the this.containerRef.offsetWidth and this.containerRef.scrollWidth can be zero when we resize window.Now this.state.isEllipsisActive can be 0:And because of that the condition here will render 0 with App/src/components/DisplayNames/index.js Lines 108 to 114 in f64a388
What changes do you think we should make in order to solve the problem?Make sure the {this.props.displayNamesWithTooltips.length > 1 && Boolean(this.state.isEllipsisActive) && (
<View style={styles.displayNameTooltipEllipsis}>
<Tooltip text={this.props.fullTitle}>
{/* There is some Gap for real ellipsis so we are adding 4 `.` to cover */}
<Text>....</Text>
</Tooltip>
</View>
)} What alternative solutions did you explore? (Optional)We can disable tooltip with mobile web |
Job added to Upwork: https://www.upwork.com/jobs/~01172d30b33b712701 |
Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
Triggered auto assignment to @Beamanator ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.mWeb - New room - Room names has 0 at end. What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
class DisplayNames extends PureComponent {
constructor(props) {
...rest code
this.checkEllipsis = this.checkEllipsis.bind(this);
}
componentDidMount() {
this.checkEllipsis() // Added function instead of direct setState call.
}
...rest code
/**
* Function - Check if Ellipsis is active and Set the class variable.
*/
checkEllipsis() {
const {scrollWidth, offsetWidth} = this.containerRef || {};
if ( !scrollWidth || !offsetWidth ) {
this.setState({isEllipsisActive: false});
}
this.setState({isEllipsisActive: scrollWidth > offsetWidth});
}
...rest code
} What alternative solutions did you explore? (Optional)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Room names in LHN do not match the name when created. Room name shows 0 at the end What is the root cause of that problem?The issue is the condition here is not robust App/src/components/DisplayNames/index.js Line 23 in 9f1c0e0
When this.containerRef.scrollWidth or this.containerRef.offsetWidth is 0 (in case of mWeb), isEllipsisActive will be 0 , not boolean as it should be, so it will show the additional 0 here App/src/components/DisplayNames/index.js Line 108 in 9f1c0e0
What changes do you think we should make in order to solve the problem?We should never chain
(or can check As we can see, all elements of the What alternative solutions did you explore? (Optional)We can cast |
@Santhosh-Sellavel bump on the proposals when you get the chance ^ thanks! |
@Beamanator |
📣 @hungvu193 You have been assigned to this job by @Beamanator! |
@Santhosh-Sellavel can you give some feedback on other proposals and why the selected proposal is the best? Since there're different solutions to the issue. Thanks! |
@tienifr @jeet-dhandha Proposed the same approach, the other approach is a bit over-engineered. @tienifr You are just changing the computation. I think that's a good option too. But instead of changing conditions, just getting a Boolean value from the final computation is simple and working so went with it. cc: @Beamanator |
@Santhosh-Sellavel It's working but:
cc @Beamanator |
That's not the case now and fairly simple usage and more or less an edge case, I can't think of any scenario where might need that. In the future when it's needed the changes will be done accordingly. Where there stated need to be computed/change state also, instead of one-time initialization. At that point, we might go with a function that computes the state and return it once. cc: @Beamanator |
Triggered auto assignment to @michaelhaxhiu ( |
This comment was marked as duplicate.
This comment was marked as duplicate.
@Natnael-Guchima @hungvu193 have been paid. @Santhosh-Sellavel can you please apply to the job and complete the BZ check list? I am going OOO until Wednesday so assigning a buddy to watch this while I am away. @michaelhaxhiu can you please pay @Santhosh-Sellavel once they apply? You can leave the BZ checklist for me until Wednesday |
@Beamanator, @michaelhaxhiu, @hungvu193, @NicMendonca, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Bump @michaelhaxhiu - are you able to get to payment today? If not, I guess @NicMendonca will be back tomorrow to finish payment (assuming @Santhosh-Sellavel applied for the job) |
@Santhosh-Sellavel accept my offer please 😄 |
@Santhosh-Sellavel I know we're holding on payment for this one, but friendly bump on the checklist: #20510 (comment) |
@Santhosh-Sellavel bump ^ |
We can skip the checklist here as it's a very minor thing, I don't think we need regression tests as well. As this should be covered by the normal test cases. If needed let's use this @Beamanator Let me know if differ on any of the above thanks! |
Regression Steps:
👍 or 👎 |
@Santhosh-Sellavel can you remind me next steps for Payment? |
Ya sure! |
Paid on New Dot |
Fab! We're all done set here then. Thanks everyone 🎉 |
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:
The name of the room matches the name when created
Actual Result:
Room names in LHN do not match the name when created. Room name shows 0 at the end
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.26.1
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
Bug6086477_Screen_Recording_20230609_090130_Chrome.mp4
Bug6086477_2023_06_09_13_46_Img_9746.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team / @Natnael-Guchima
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: