[HOLD for payment 2023-12-20] [$500] Room - Long workspace name is not truncated in workspace selection page during room creation

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.7-0 Reproducible in staging?: Y Reproducible in production?: Y 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Precondition: Have a long workspace name

1. Go to staging.new.expensify.com
2. Click + > New chat > Room
3. Click Workspace

Expected Result:

Long workspace name will be truncated

Actual Result:

Long workspace name is not truncated

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android: Native
• [x] Android: mWeb Chrome
• [ ] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/2efdd910-ad58-4d70-9666-3aba6be49cbc

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cf730d76f7807ef8
• Upwork Job ID: 1730706237151584256
• Last Price Increase: 2023-12-01
• Automatic offers:
• s77rt | Reviewer | 27955714
• abzokhattab | Contributor | 27955717

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01cf730d76f7807ef8

[melvin-bot[bot]]

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

[abzokhattab]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Long workspace name is not truncated in workspace selection page during room creation

What is the root cause of that problem?

1. the workspace field uses the ValuePicker component to render items selections:

https://github.com/Expensify/App/blob/d181fd7ed23427f01d591a8302e46cb79e2dadb0/src/pages/workspace/WorkspaceNewRoomPage.js#L217-L223

2. The ValuePicker relies on ValueSelectorModal, which, in turn, uses a selection list to render items.

https://github.com/Expensify/App/blob/d521dd6783c382297e06a429701ae1936f808041/src/components/ValuePicker/ValueSelectorModal.js#L67-L72

3. The selection list uses BaseSelectionList with a renderItem function, which employs BaseListItem to render each item. https://github.com/Expensify/App/blob/6cf91f61cb339c984b1a97ff79af8659ccef128f/src/components/SelectionList/BaseSelectionList.js#L303-L313

4. the BaseListItems uses RadioListItem in our case: https://github.com/Expensify/App/blob/de53c93cedbc0a261fe28909e68a1a928602052a/src/components/SelectionList/BaseListItem.js#L32

5. The RadioListItem sets the text styles for the displayed items. https://github.com/Expensify/App/blob/d521dd6783c382297e06a429701ae1936f808041/src/components/SelectionList/RadioListItem.js#L12-L15

The problem arises because the Text component used in RadioListItem does not have the styling needed to truncate item labels.

What changes do you think we should make in order to solve the problem?

since there is no tooltip in the /settings/workspaces list page, then we dont need to enable the tooltip here, we only need to truncate the text by changing the style to:

   <Text
                style={[styles.optionDisplayName, isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, item.isSelected && styles.sidebarLinkTextBold, styles.pre]}
                numberOfLines={1}
            >

POC:

Note: if we dont want to apply this truncation on every selection list then we can create a new boolean prop truncateText in the selectionList and set it to true whenever we want to truncate any list

[DylanDylann]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• Room - Long workspace name is not truncated in workspace selection page during room creation

What is the root cause of that problem?

• Currently, the the workspace selection page is using RadioListItem to display its items.
• And it does not have the logic to truncate the text in case of long text.

  What changes do you think we should make in order to solve the problem?

• We need to handle the case of the long text: It should truncate the long text and if user hover over the elipsis, it will display the full text as we did in other sections. Can do it by updating: https://github.com/Expensify/App/blob/1682be47d71b5a84c20c2396e833c0a015829252/src/components/SelectionList/RadioListItem.js#L11 to:

          <DisplayNames
              fullTitle={item.text}
              tooltipEnabled
              numberOfLines={1}
              textStyles={[styles.optionDisplayName, isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText, item.isSelected && styles.sidebarLinkTextBold]}
              shouldUseFullTitle
          />

• And we also need to handle the alternativeText section as we did above: https://github.com/Expensify/App/blob/1682be47d71b5a84c20c2396e833c0a015829252/src/components/SelectionList/RadioListItem.js#L13-L15

What alternative solutions did you explore? (Optional)

• NA

Result:

Screencast from 02-12-2023 11:08:50.webm

[ishpaul777]

I reported this as bug when bugs were accepted from contributors but this was never logged to GH, @NicMendonca Can you check on slack this will be the last bug report from @ishpaul in #expensify-bugs channel and add me as reporter

[s77rt]

@abzokhattab Thanks for the proposal. Your RCA is correct. Limiting the number of lines (i.e. numberOfLines={1}) makes sense to me. However you also mentioned the need to change the style (adding styles.pre). Can you please elaborate why this is needed?

[DylanDylann]

@s77rt Should we need to display the full name once user hover over the ellipsis like we did in other sections?

[s77rt]

@DylanDylann Thanks for the proposal. Your RCA is correct. Regarding the tooltip, is there a specific reason to use <DIsplayNames /> instead of just <Tooltip />?

[DylanDylann]

@s77rt The DisplayName component have been used in a lot of sections in our app, so I think we should use it for consistency and reducing the duplicate logic

[abzokhattab]

However you also mentioned the need to change the style (adding styles.pre). Can you please elaborate why this is needed?

Correct the pre style is not needed to make it work but I added it to make it consistent with the DisplayNamesWithoutTooltip component:

https://github.com/Expensify/App/blob/d521dd6783c382297e06a429701ae1936f808041/src/components/DisplayNames/DisplayNamesWithoutTooltip.tsx#L17-L27

pre styling is used for border styling, background color, padding, a monospace font, and removes any margins from the top and bottom of the text block

if we are okay with that an alternative would be to use DisplayNamesWithoutTooltip, but i didn't want to change the Text component to ensure backward combinability and avoid potential regressions so again its not needed

[s77rt]

@DylanDylann For this issue I don't think using DisplayNames is a critical factor for proposal selection since we are still dealing with the same solution (which is to add numberOfLines prop). Also RadioListItem is a core component where it could be better not to use DisplayNames since it involves unnecessary logic. One last argument is that RadioListItem is a generic component where DisplayNames is only designed for accounts.

[s77rt]

@abzokhattab I think we can keep the styles.pre not to collapse multiple whitespaces and to be consist with the selected value

Selected value:

RadioListItem without styles.pre

RadioListItem with styles.pre

[s77rt]

We can proceed with @abzokhattab's proposal.

🎀 👀 🎀 C+ reviewed Link to proposal

[melvin-bot[bot]]

Triggered auto assignment to @iwiznia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[DylanDylann]

@s77rt

1. since we are still dealing with the same solution (which is to add numberOfLines prop)

• By using DisplayName, the user can see the full workspace name once they hover over the ellipsis. I think we need to confirm with the team about this. This is the core different between my proposal and the other one

  2. One last argument is that RadioListItem is a generic component where DisplayNames is only designed for accounts.

• I don't think DisplayNames is only designed for accounts because it also used in MenuItem

• Also, the DisplayName is just used to display the long text with the hoverable ellipsis, does not anchor to the particular use case

• If you worry about backward combinability, we can also add the prop named shouldUseDisplayName and just use the DislayName if shouldUseDisplayName: true

[s77rt]

@DylanDylann For the tooltip we can just use <Tooltip /> to be consist with <UserListItem />. The SearchPage will be refactored ^1 to use the SelectionList and inside we will be using UserListItem as well instead of DisplayNames.

[DylanDylann]

@s77rt Maybe there is a misunderstood here. I mean that we use the <DisplayName /> to handle the feature: If the text is too long, it will display as 'this is a long text ...' (a text with a elipsis ... at the end). So once user hover over this elipsis ..., it will display the full text

[DylanDylann]

But I see that your mean is the feature: Hovering over the text will display the text in the tooltip. Please correct me, thanks

[s77rt]

@DylanDylann Hovering the text would display the tooltip in the same way we do with UserListItem (currently used in Workspace invite page, and will be used on the referenced SearchPage)

[DylanDylann]

@s77rt No. I mean hover over the ..., not hover over the text

[abzokhattab]

My thinking reasoning was that there is no tooltip on the /settings/workspaces workspace list page so that's why I think we should stay consistent with it:

[s77rt]

@DylanDylann I think https://expensify.slack.com/archives/C01GTK53T8Q/p1701561828595899 answered your concern.

[DylanDylann]

@s77rt Yeah. Thanks for your information

[s77rt]

@abzokhattab Having a tooltip or not is not a blocker. We can proceed with fixing the reported bug and add a tooltip if it's needed.

[iwiznia]

Note: if we dont want to apply this truncation on every selection list then we can create a new boolean prop truncateText in the selectionList and set it to true whenever we want to truncate any list

Hmmmm why wouldn't we want to make this component always truncate and always have the hover text (at least when it is truncated)? Displaying a list with cut out names is not a feature we would ever want I would think, no?

[s77rt]

@iwiznia It will always truncate the name and if we add the tooltip it will always show up on text hover.

[iwiznia]

Ah ok perfect then!

[melvin-bot[bot]]

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @abzokhattab 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.11-25 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/32532

If no regressions arise, payment will be issued on 2023-12-20. :confetti_ball:

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.

• [ ] External issue reporter
• [ ] Contributor that fixed the issue
• [ ] Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @s77rt requires payment automatic offer (Reviewer)
• @abzokhattab requires payment automatic offer (Contributor)

[melvin-bot[bot]]

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:

• [ ] [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [ ] [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [ ] [@s77rt] Determine if we should create a regression test for this bug.
• [ ] [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [ ] [@NicMendonca] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: https://github.com/Expensify/App/pull/27767
• The offending PR has been commented on: https://github.com/Expensify/App/pull/27767#discussion_r1425904335
• A discussion in #expensify-bugs has been started: https://expensify.slack.com/archives/C049HHMV9SM/p1702503250937009
• Determine if we should create a regression test for this bug: No

[ishpaul777]

gentle bump @NicMendonca for https://github.com/Expensify/App/issues/32384#issuecomment-1837119885, incase you missed it

[NicMendonca]

@lanitochka17 can you confirm this please?

@ishpaul777 this is the last bug I see reported from you in that channel

[ishpaul777]

Hmm..., I am confident i reported it as bug may be a day or 2 days before bugs channel disappeared but it was not logged, Can you try searching @Ishpaul in:#expensify-bugs filter-Newest ( its set to Most relevant by default) may be, its fine if you can't find it maybe i am mistaken

[NicMendonca]

@ishpaul777 Are you able to search for that yourself and post it here?

I am not finding anything recent from you or related to this:

[ishpaul777]

actually you should not search from:@ishpaul singh bugs are reported by BOT, so i believe the correct search is @ishpaul

I dont have bug channel so i cant see it :(

[s77rt]

I found it https://expensify.slack.com/archives/C049HHMV9SM/p1698155075411239

[NicMendonca]

@ishpaul777 can you apply to the job please? https://www.upwork.com/jobs/~01cf730d76f7807ef8

[NicMendonca]

@s77rt @abzokhattab - you've both been paid via Upwork

[ishpaul777]

@NicMendonca Applied! Thanks 😀

[NicMendonca]

@ishpaul777 you've been paid

[NicMendonca]

we're all set here!