Open lanitochka17 opened 5 days ago
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.
Room title line is broken on room details page.
The line for room title is broken here.
We can use reportName
for StringUtils.lineBreaksToSpaces(reportName)
N/A
It is expected that we truncate in the header, but we are supposed to show the full name in room details.
Job added to Upwork: https://www.upwork.com/jobs/~021859338322732202800
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External
)
This is a pretty minor bug, so demoting
Upwork job price has been updated to $125
@Shahidullah-Muffakir Your proposal will be dismissed because you did not follow the proposal template.
Room name is truncated in Room Details page.
Improvement Now the default numberOfLinesTitle 1 is applied
here pass a new prop for numberOfLinesTitle
,
like:
numberOfLinesTitle={5}
Edited by proposal-police: This proposal was edited at 2024-11-21 11:29:27 UTC.
Room name displayed truncated on header and details when using long name
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
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:
const combinedTitleTextStyle = StyleUtils.combineStyles(
[
.........
styles.breakWord,
],
titleStyle ?? {},
);
We did the same thing in previous PR's as well:
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
We can increase the character limit i.e to 200
or 150
Or we can remove this prop
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
Triggered auto assignment to @chiragsalian, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
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 I have tested it. You can also check out the docs to see that 0
is supported.
@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
Room name has a character limit of 100. Considering this, setting numberOfLinesTitle={5}
should be sufficient.
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:
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:
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.
~FYI this is a chrome specific issue i guess, the expected result is matched well on safari:~
sorry investigating
@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:
c.c. @chiragsalian
Android - Room - Room name displayed truncated on header and details when using long name
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
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
Optionally we can set the numberOfLinesTitle to higher value that matches the room name length limit
@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 ๐
We are fixing for all type of reports except for expense reports @FitseTLT
@FitseTLT Please explain, preferably with screenshots, in what way the proposal would break group chats.
@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
@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).
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?
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
Yep, that makes sense. My concern is more with the default name. Group chats can be quite large. If we don't truncate the name at all and a group uses the default name and it has let's say 100 members, would we show all of them on the details page? That would be quite long...
Whereas a custom name is going to be capped by the character limit.
Yep, that makes sense. My concern is more with the default name. Group chats can be quite large. If we don't truncate the name at all and a group uses the default name and it has let's say 100 members, would we show all of them on the details page? That would be quite long...
Whereas a custom name is going to be capped by the character limit.
Yep. That's why I saying we should only apply the change for rooms ๐
Yep, that makes sense. My concern is more with the default name. Group chats can be quite large. If we don't truncate the name at all and a group uses the default name and it has let's say 100 members, would we show all of them on the details page? That would be quite long...
Whereas a custom name is going to be capped by the character limit.
I think passing numberOfLinesTitle={5}
will work for these cases as well.
would we show all of them on the details page? That would be quite long...
No we won't.
@joekaufmanexpensify that won't be a problem with my proposal, because we limit group names to 100 characters (so even when we have 100 members, only the name would be upto 100 characters), so all the others. @jjcoffee i did test with groups as well
Whereas a custom name is going to be capped by the character limit.
Default as well as custom are capped at 100
c.c. @jjcoffee
Here's the proof too:
https://github.com/user-attachments/assets/f51d46b3-c0f7-4502-8e3b-fc393894a64b
So overall, we can safely remove the truncation for all room types in this case @joekaufmanexpensify with no side effects from that
c.c. @chiragsalian as well, since the C+ selected my proposal and we cleared that we want the name to be whole for all type of rooms
@shawnborton What would be your design perspective on this issue?
@FitseTLT can you summarize what decisions you need help making?
@FitseTLT can you summarize what decisions you need help making?
According to the OP we were aiming to display the full room name in report details page room name Menu item like
First do you agree with that? Then came another point whether we should apply the same behaviour for all reports such as for group chats like
WDYT about that too
My opinion,
Those are my thoughts but I'll let @shawnborton weigh in from a design perspective. Let me know if you've understood where we're at and if not i can summarize again.
This is because group names don't have the same 100 character limit unless you edit it
@chiragsalian if you check the video here, there is a 100 character limit for default group name as well
The default as well as custom group name have a limit of 100 characters
if you check the video https://github.com/Expensify/App/issues/52836#issuecomment-2494174257, there is a 100 character limit for default group name as well
No i don't think that's proof. Feel free to correct me if i am mistaken. So here I have a group report with this group name,
chirag.exp.test1@gmail.com, chirag.exp.test1+a@gmail.com, chirag.exp.test1+b@gmail.com, chirag.exp.test1+c@gmail.com, chirag.exp.test1+d@gmail.com, chirag.exp.test1+e@gmail.com, chirag.exp.test1+f@gmail.com, chirag.exp.test1+g@gmail.com, chirag.exp.test1+h@gmail.com, chirag.exp.test1+i@gmail.com, chirag.exp.test1+j@gmail.com, chirag.exp.test1+k@gmail.com, chirag.exp.test1+l@gmail.com, chirag.exp.test1+m@gmail.com, test
Thats 400+ characters and i see it fully here,
If we show the same without a character limit in the report details screen it will be very long. Only once i edit it will it show me that i have to reduce the number of characters to 100.
@chiragsalian We only show the first 5 emails of the participants for group chats in details page that's what is tricking @twilight2294 https://github.com/Expensify/App/blob/2b312769d1bb8b621b30a11be26fddcbd4b9c738/src/libs/ReportUtils.ts#L2269-L2270 but that still can surpass 100 chars if the logins are very long.
I don't think I feel too strongly. I think the reality is that most room name aren't actually going to be that long. But I do like the idea of being consistent with room/group names everywhere.
Ah, if we're only showing the first 5 logins in the default group chat name anyway, then that alleviates my concern. I agree it'd be nice to do the same thing with rooms/group names.
Sure, go for it. Feel free to create the PR.
๐ฃ @twilight2294 ๐ 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 ๐
PR ready for review @jjcoffee
PR in review
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: 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:
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?
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
Issue Owner
Current Issue Owner: @jjcoffee