[HOLD for payment 2023-08-03] [$1000] NEW FEATURE: Don't truncate long room names in room details page

[kavimuru]

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:

1. Click Global create > new room, and enter a long room name (more than 50 characters).
2. Notice that the full room name is listed in the welcome message, and truncated in the room header.
3. Click into room details and notice that the room name in room details is also truncated.

Expected result:

We'd show the full room name in room details, as this should be the source of truth for what the room actually is. If the room name is long, we should just break it into multiple lines in the room details.

Actual result:

We truncate the room name in room details. Meaning you cannot see the full room name there.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number:
Reproducible in staging?:
Reproducible in production?:
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: @joekaufmanexpensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689026712373409

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f72d5afba7910b13
• Upwork Job ID: 1678728351340113920
• Last Price Increase: 2023-07-11

[melvin-bot]

Triggered auto assignment to @jliexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

[jliexpensify]

@shawnborton let me know if you'd like this as External!

[dhairyasenjaliya]

Proposal

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

• Don't truncate long room names in room details page

What is the root cause of that problem?

• New Feature request 


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

• We need to define max number of line to display on room detail page for consistency as we have set max room length of 79 We can ensure to add max number of line to 4 which we can decide but I prefer 4 as it display full room name

• Also if we need only for room then we need to add conditional check and set numberOfLines set to custom only if chat type is room

• We need to add custom numberOfLine to ReportDetailsPage.js inside that we have used <DisplayNames>

Note: Currently on this GH it mentioned only for native iOS and Android so that is the reason I have not added changes for the web if needed we need to adjust pre style only

Result

cc @shawnborton

[Pujan92]

Proposal

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

Don't truncate the room name in the details page which help user to see full room name

What is the root cause of that problem?

Currently, we are passing numberOfLines to 1 for showing the title which truncates the text and only shows a single line.

App/src/pages/ReportDetailsPage.js

Line 153 in 38890e2

numberOfLines={1}

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

1. We only need to set the numberOfLines to 1 when it is not a chat room. Setting to 0 will result in unsetting this value, which means that no lines restriction will be applied.
   numberOfLines={isChatRoom ? 0 : 1}
   

   App/src/pages/ReportDetailsPage.js

   Line 153 in 38890e2

   numberOfLines={1}

2. To allow it to break in multiple lines we also need to take out the style styles.pre from here.
   

   App/src/pages/ReportDetailsPage.js

   Line 154 in 38890e2

   textStyles={[styles.textHeadline, styles.textAlignCenter, styles.pre]}

   
   isChatRoom ? undefined : styles.pre

Note: It is applied to all platforms, if requires only for small devices then we need to add withWindowDimensions and with isChatRoom condition in above 2 points we also need to add && props.isSmallScreenWidth.

Result

[shawnborton]

Yup we can do this external, thanks!

[melvin-bot]

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

[melvin-bot]

Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[mananjadhav]

Thanks for the proposals folks. For consistency, I think we should focus on solving for all the platforms and not just native apps.

Both the proposals don't fully solve for all the platforms. @dhairyasenjaliya @Pujan92 Can you update your proposals after testing once? Looking forward to it. Also I would recommend sending out a fresh comment, so that we don't end up in a conflict of which proposal came first. Thank you for your patience on this.

[Pujan92]

@mananjadhav my proposal is actually a generic one that behaves the same for all platforms.

[mananjadhav]

@Pujan92 still truncated on web using your proposal.

--- a/src/pages/ReportDetailsPage.js
+++ b/src/pages/ReportDetailsPage.js
@@ -150,8 +150,8 @@ function ReportDetailsPage(props) {
                                     fullTitle={ReportUtils.getReportName(props.report)}
                                     displayNamesWithTooltips={displayNamesWithTooltips}
                                     tooltipEnabled
-                                    numberOfLines={1}
-                                    textStyles={[styles.textHeadline, styles.textAlignCenter, styles.pre]}
+                                    numberOfLines={isChatRoom ? 0 : 1}
+                                    textStyles={[styles.textHeadline, styles.textAlignCenter, isChatRoom ? undefined : styles.pre]}
                                     shouldUseFullTitle={isChatRoom || isPolicyExpenseChat || isThread}
                                 />
                             </View>

[dhairyasenjaliya]

Proposal

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

• Don't truncate long room names in room details page

What is the root cause of that problem?

• New Feature request 


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

• We need to define max number of line to display on room detail page for consistency as we have set max room length of 79 We can ensure to add max number of line to 4 which we can decide but I prefer 4 as it display full room name

• IMO we do not need to add 0 as numberOfLine as we already aware we have limit of 79 and I did test run on smaller devices as well max line was 4 so for that reason we should add max to 4 rather than 0

• Also if we need only for room then we need to add conditional check and set numberOfLines set to custom only if chat type is room

• We need to add a custom numberOfLine to ReportDetailsPage.js inside that we have used <DisplayNames>

• The above changes do resolve on native iOS and Android but for web and mWeb we have added styles.pre that wraps text to line 1 so if we have a chat type of room then we just remove styles.pre

• I have not added web changes just because this GH was mentioned only for Native iOS and Android otherwise for web there are only style changes to it

Result

cc @mananjadhav

[mananjadhav]

Both of you mention about removing the styles.pre, but that shouldn't break #15646

[dhairyasenjaliya]

@mananjadhav shouldn't be a problem for room name as we don't allow space inside it and add - instead of space so i think we are good

[mananjadhav]

But the display name component is component for room as well as chats, won't that cause an issue?

[Pujan92]

Proposal

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

Don't truncate the room name in the details page which help user to see full room name

What is the root cause of that problem?

Currently, we are passing numberOfLines to 1 for showing the title which truncates the text and only shows a single line.

App/src/pages/ReportDetailsPage.js

Line 153 in 38890e2

numberOfLines={1}

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

1. We only need to set the numberOfLines to 1 when it is not a chat room. Setting to 0 will result in unsetting this value, which means that no lines restriction will be applied.
   numberOfLines={isChatRoom ? 0 : 1}
   

   App/src/pages/ReportDetailsPage.js

   Line 153 in 38890e2

   numberOfLines={1}

2. To allow it to break in multiple lines we also need to take out the style styles.pre from here.
   

   App/src/pages/ReportDetailsPage.js

   Line 154 in 38890e2

   textStyles={[styles.textHeadline, styles.textAlignCenter, styles.pre]}

   
   isChatRoom ? undefined : styles.pre

3. Update numberOfLines=1 with numberOfLines=this.props.numberOfLines in the DisplayNames to take the dynamic value from parent.
   

   App/src/components/DisplayNames/index.js

   Line 85 in f2d38d4

   numberOfLines={1}

Result

[dhairyasenjaliya]

Yeah thats what suggestions is to remove pre only if we have chat type is room other than that we should apply pre since we allow space on other display name

[Pujan92]

But the display name component is component for room as well as chats, won't that cause an issue?

Only for Room we are taking out the whiteSpace: pre style so I think it won't create any issue.

[mananjadhav]

Thanks for the comments folks. Yeah I saw that on @Pujan92's proposal but I missed reading that in yours @dhairyasenjaliya.

Considering both the proposals, I think @Pujan92's proposal is better to unset the numberOfLines, instead of hard-coding it.

I am a bit concerned about the comment, // Tokenization of string only support 1 numberOfLines on Web, so we'll have to test that we don't break something and trace it back to the original PR when this comment was added.

🎀 👀 🎀 C+ Reviewed.

[b4s36t4]

I'm just wondering passing 0 as numberOfLines does make any sense? isn't it weird to understand we're saying 0 lines and it will show all the lines of text? why not just pass undefined or null this makes somewhat sense in my view (came here after seeing PR)?

[melvin-bot]

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

[melvin-bot]

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

• Fix: Don't truncate long room names in room details page #23357

If no regressions arise, payment will be issued on 2023-08-03. 🎊

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:

• @mananjadhav does not require payment (Eligable for Manual Requests)
• @Pujan92 requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[mananjadhav]

This is more like a feature request. Hence we don't have any tagged PR. Because we shouldn't end up working on adding the limit again, I think we should add a regression test.

Regression Test:

1. Open FAB, and click on New Room
2. Create a room with a very long name (> 100 characters)
3. Open the room chat
4. Click on room name in the header to open report details
5. Verify that room name won't truncate and shows entire room name text

@Julesssss @jliexpensify wdyt?

Also this is eligible for the timeline bonus, because the change was done well within the timeline, but we got blocked by another PR conflict.

[jliexpensify]

Will let @Julesssss weigh in with the final say on timeline bonus - I'll summarise payments once he weighs in!

[Julesssss]

Regular payment here @jliexpensify

[jliexpensify]

Payments:

• Contributor: @Pujan92 $1000 + $500 (speed bonus)
• C+: @mananjadhav $1000 + $500 (speed bonus)

Upworks job here

[mananjadhav]

@jliexpensify It should be $1500, but let's wait for @Julesssss to ack.

[Julesssss]

@jliexpensify It should be $1500, but let's wait for @Julesssss to ack.

Why is that?

[mananjadhav]

@Julesssss I raised this comment here too during the PR. We had the PR ready but had conflicts. The conflict was primarily a regression, which got this issue delayed. I saw your 👍 in the comment as well as I highlighted the same [here too].(#22613 (comment))

[Julesssss]

Ah yeah, I remember now. 👍

[mananjadhav]

Thanks. Can you comment and confirm it to @jliexpensify so that he can put up a summary on the issue?

[Julesssss]

@jliexpensify I hereby declare this is worthy of the bonus 😄

[jliexpensify]

Edited

[mananjadhav]

Raised my request on the NewDot.

[jliexpensify]

Pujan paiud, job closed. Working on Regression GH now.

[JmillsExpensify]

Reviewed details for @mananjadhav. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.