App icon indicating copy to clipboard operation
App copied to clipboard

[$125] Android - Room - Room name displayed truncated on header and details when using long name

Open lanitochka17 opened this issue 1 year ago • 54 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.64-4 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Open the Expensify app.
  2. Tap on FAB and select "Create Chat"
  3. Tap on "Room"
  4. Create a long name for the room (More than 50 characters) and save it
  5. Verify that the name is displayed completely in header and on room details page

Expected Result:

Room name should be displayed in full in header and on details page, despite using a name with more than 50 characters

Actual Result:

Room name in header and in details is truncated when creating a long room name. (More than 50 characters)

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/09c337be-5c38-4769-b8dd-07b565e3be4a

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859338322732202800
  • Upwork Job ID: 1859338322732202800
  • Last Price Increase: 2024-11-20
  • Automatic offers:
    • twilight2294 | Contributor | 105029064
Issue OwnerCurrent Issue Owner: @joekaufmanexpensify

lanitochka17 avatar Nov 20 '24 18:11 lanitochka17

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Nov 20 '24 18:11 melvin-bot[bot]

Proposal

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

Room title line is broken on room details page.

What is the root cause of that problem?

https://github.com/Expensify/App/blob/b7fdace7fd14c18e314e008554f07ad35369db6b/src/pages/ReportDetailsPage.tsx#L740

The line for room title is broken here.

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

We can use reportName for StringUtils.lineBreaksToSpaces(reportName)

What alternative solutions did you explore? (Optional)

N/A

jacobkim9881 avatar Nov 20 '24 20:11 jacobkim9881

It is expected that we truncate in the header, but we are supposed to show the full name in room details.

joekaufmanexpensify avatar Nov 20 '24 20:11 joekaufmanexpensify

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

melvin-bot[bot] avatar Nov 20 '24 20:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 20 '24 20:11 melvin-bot[bot]

This is a pretty minor bug, so demoting

joekaufmanexpensify avatar Nov 20 '24 20:11 joekaufmanexpensify

Upwork job price has been updated to $125

melvin-bot[bot] avatar Nov 20 '24 20:11 melvin-bot[bot]

@Shahidullah-Muffakir Your proposal will be dismissed because you did not follow the proposal template.

github-actions[bot] avatar Nov 20 '24 22:11 github-actions[bot]

Proposal

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

Room name is truncated in Room Details page.

What is the root cause of that problem?

Improvement Now the default numberOfLinesTitle 1 is applied

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

https://github.com/Expensify/App/blob/0c99ad07fef6eb83eba46d3cab071ca395d401bf/src/pages/ReportDetailsPage.tsx#L737

here pass a new prop for numberOfLinesTitle, like: numberOfLinesTitle={5}

What alternative solutions did you explore? (Optional)

Shahidullah-Muffakir avatar Nov 20 '24 22:11 Shahidullah-Muffakir

Edited by proposal-police: This proposal was edited at 2024-11-21 11:29:27 UTC.

Proposal

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

Room name displayed truncated on header and details when using long name

What is the root cause of that problem?

In MenuItemWithTopDescription, we do not pass value for numberOfLinesTitle, so it will default to 1: https://github.com/Expensify/App/blob/0c99ad07fef6eb83eba46d3cab071ca395d401bf/src/pages/ReportDetailsPage.tsx#L737-L738 https://github.com/Expensify/App/blob/0c99ad07fef6eb83eba46d3cab071ca395d401bf/src/components/MenuItem.tsx#L362

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

If we do not want to truncate the room name at all we should pass numberOfLinesTitle={0} in MenuItemWithTopDescription, this makes sure that there is no hardcoded limit on the number of lines the title can have.

We would also need to update the combinedTitleTextStyle in MenuItem, we need to break the line if the word doesn't fit the given line:

https://github.com/Expensify/App/blob/00a81dc63f265a9f90a29f2d744a70465924f7f1/src/components/MenuItem.tsx#L461

    const combinedTitleTextStyle = StyleUtils.combineStyles(
        [
.........
            styles.breakWord,
        ],
        titleStyle ?? {},
    );
    

We did the same thing in previous PR's as well:

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

What alternative solutions did you explore? (Optional)

twilight2294 avatar Nov 20 '24 22:11 twilight2294

Proposal

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

What is the root cause of that problem?

Right here we pass shouldTruncateTitle true and set the character limit to 100 https://github.com/Expensify/App/blob/c18812ae3cac59025da0f6c8657750dfcb779a65/src/pages/ReportDetailsPage.tsx#L846-L852

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

We can increase the character limit i.e to 200 or 150

Or we can remove this prop

https://github.com/Expensify/App/blob/0c99ad07fef6eb83eba46d3cab071ca395d401bf/src/pages/ReportDetailsPage.tsx#L851-L852

What alternative solutions did you explore? (Optional)

NJ-2020 avatar Nov 21 '24 04:11 NJ-2020

Let's go with @twilight2294's proposal. We don't want any truncation so it makes sense to make the numberOfLinesTitle unlimited.

The first proposal has the correct RCA, but adds a limit on the numberOfLinesTitle. The last proposal refers to a part of the code that doesn't relate to the room title.

:ribbon::eyes::ribbon: C+ reviewed

jjcoffee avatar Nov 21 '24 09:11 jjcoffee

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

melvin-bot[bot] avatar Nov 21 '24 09:11 melvin-bot[bot]

Let's go with @twilight2294's proposal. We don't want any truncation so it makes sense to make the numberOfLinesTitle unlimited.

The first proposal has the correct RCA, but adds a limit on the numberOfLinesTitle. The last proposal refers to a part of the code that doesn't relate to the room title.

🎀👀🎀 C+ reviewed

@jjcoffee I don't think passing numberOfLinesTitle={0} will work, can we test it please? and as I mentioned, I gave 5 as an example we can increase the number of lines.

Shahidullah-Muffakir avatar Nov 21 '24 09:11 Shahidullah-Muffakir

@Shahidullah-Muffakir I have tested it. You can also check out the docs to see that 0 is supported.

jjcoffee avatar Nov 21 '24 09:11 jjcoffee

@jjcoffee , maybe I am missing something because when I pass numberOfLinesTitle={0} , this is the output, where the complete name of the room is not shown

Screenshot 2024-11-21 at 3 16 21 PM

Shahidullah-Muffakir avatar Nov 21 '24 09:11 Shahidullah-Muffakir

Screenshot 2024-11-21 at 3 36 04 PM

twilight2294 avatar Nov 21 '24 10:11 twilight2294

https://github.com/user-attachments/assets/3111a1f0-81de-4595-a34c-8f2f3f4b29ce

Shahidullah-Muffakir avatar Nov 21 '24 10:11 Shahidullah-Muffakir

Room name has a character limit of 100. Considering this, setting numberOfLinesTitle={5} should be sufficient.

https://github.com/Expensify/App/blob/376f9f08755b0555cf3666c5a880542af57c7da1/src/pages/settings/Report/RoomNamePage.tsx#L73

Shahidullah-Muffakir avatar Nov 21 '24 10:11 Shahidullah-Muffakir

Oh yes you're right that on Chrome web it's not displaying correctly (it does work on Android though). I believe there's probably a style fix for that, since it works in the example given for the TaskView description:

https://github.com/Expensify/App/blob/e3331db018e826dc20bb34ad33b3fb77ee6e8fb8/src/components/ReportActionItem/TaskView.tsx#L141-L152

Having said that since the room name has a char limit, we could decide on a numberOfLines that makes sense, although this has the downside that if we ever increase the char limit, we'd have to remember to update numberOfLines. So I still slightly lean towards making it unlimited and fixing the style.

We do set a limit for the TaskView title here though:

https://github.com/Expensify/App/blob/e3331db018e826dc20bb34ad33b3fb77ee6e8fb8/src/components/ReportActionItem/TaskView.tsx#L117-L124

I'll leave it to @chiragsalian to make the final decision. I must say the level of detail in the proposal that has it as a solution is pretty unacceptable though, especially for such a simple issue.

jjcoffee avatar Nov 21 '24 10:11 jjcoffee

~FYI this is a chrome specific issue i guess, the expected result is matched well on safari:~

sorry investigating

twilight2294 avatar Nov 21 '24 10:11 twilight2294

@jjcoffee I got what was happening here, there was no word break here, so even when we were allowing multi-lines then too for a single word long enough it wasn't going to the new line, I have fixed that and also updated the proposal:

Screenshot 2024-11-21 at 4 59 52 PM

c.c. @chiragsalian

twilight2294 avatar Nov 21 '24 11:11 twilight2294

Proposal

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

Android - Room - Room name displayed truncated on header and details when using long name

What is the root cause of that problem?

We are defaulting to numberOfLinesTitle value of 1 as we are not passing numberOfLinesTitle prop here https://github.com/Expensify/App/blob/0c99ad07fef6eb83eba46d3cab071ca395d401bf/src/pages/ReportDetailsPage.tsx#L737-L738

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

For only room chats isUserCreatedPolicyRoom we should pass numberOfLinesTitle of 0

                    numberOfLinesTitle={isUserCreatedPolicyRoom ? 0 : 1}

and set breakWord style in menu item if the numberOfLinesTitle is zero here https://github.com/Expensify/App/blob/2d2555b173b47c0bce0b1c16b275f1f98639664c/src/components/MenuItem.tsx#L468

            numberOfLinesTitle === 0 && styles.breakWord,

We can optionally pass down the breakWord title style from the MenuItemWithTopDescription for cases of room or pass some boolean param shouldBreakWord and add the style when it is true only for our room case

What alternative solutions did you explore? (Optional)

Optionally we can set the numberOfLinesTitle to higher value that matches the room name length limit

FitseTLT avatar Nov 21 '24 12:11 FitseTLT

@jjcoffee Can you please check my proposal you are choosing a wrong proposal that would unnecessarily break the menu truncation and break word behaviour of other reports too like group chat. We only want to apply the changes to rooms 👍

FitseTLT avatar Nov 21 '24 12:11 FitseTLT

We are fixing for all type of reports except for expense reports @FitseTLT

twilight2294 avatar Nov 21 '24 12:11 twilight2294

@FitseTLT Please explain, preferably with screenshots, in what way the proposal would break group chats.

jjcoffee avatar Nov 21 '24 12:11 jjcoffee

@jjcoffee It will remove the truncation for group chats we should apply the changes only for rooms according to https://github.com/Expensify/App/issues/52836#issuecomment-2489516532

image

FitseTLT avatar Nov 21 '24 12:11 FitseTLT

@joekaufmanexpensify Any thoughts on this? From the issue you linked to:

We'd show the full room name in room details, as this should be the source of truth for what the room actually is.

I would think this reasoning would apply to other areas (to have a source of truth for the full name).

jjcoffee avatar Nov 21 '24 13:11 jjcoffee

Is the question whether we should apply the same solution to group chat names in addition to rooms names on the details page?

If so, the default group chat name is just a list of all the members, right?

joekaufmanexpensify avatar Nov 21 '24 17:11 joekaufmanexpensify

Is the question whether we should apply the same solution to group chat names in addition to rooms names on the details page?

If so, the default group chat name is just a list of all the members, right?

@joekaufmanexpensify It can also be set to any custom name

FitseTLT avatar Nov 21 '24 19:11 FitseTLT