Skip to content
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

Fix: Long display name not being wrapped #3830

Merged
merged 4 commits into from
Jul 1, 2021
Merged

Fix: Long display name not being wrapped #3830

merged 4 commits into from
Jul 1, 2021

Conversation

rdjuric
Copy link
Contributor

@rdjuric rdjuric commented Jun 30, 2021

Details

Fixes an issue where our display name wasn't respecting the size of it's container.

Fixed Issues

#3172

Tests

  1. Wrote a long display name (first+last name, or email)
  2. It was trimmed correctly on the chat and on the IOU details modal

QA Steps

  1. Choose a long display name (first+last name, or email)
  2. It should be trimmed correctly on the chat and on the IOU details modal

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Web.mov

Mobile Web

mWeb.mp4

Desktop

desktop.mov

iOS

iOS.mp4

Android

Android.mp4

@rdjuric rdjuric requested a review from a team as a code owner June 30, 2021 17:49
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team June 30, 2021 17:49
@rdjuric rdjuric changed the title fixes long name Fix: Long display name not being wrapped Jun 30, 2021
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok after thinking about this, I'm not sure if this is the best result. It works great, and your testing shows that, but I'm worried that if we ever want to use a ReportActionItemFragment with fragment.type = TEXT in a different context, we might be bummed about being restricted to one line.

@marcaaron, do you think this is pre-optimizing? Should we cross that bridge if/when we get to it, or do you think this concern is worth addressing now?

@marcaaron
Copy link
Contributor

Yeah I agree @Luke9389. This solution will most likely go out of date as soon as we start adding other kinds of report actions to NewDot. Maybe better to pass this in as a prop to ReportActionItemFragment for now and then conditionally set the single line. Then apply it here where we are building the "person" text:

https://github.com/Expensify/Expensify.cash/blob/b330b3da015703a0e3ceacfb3c946bf565019752/src/pages/home/report/ReportActionItemSingle.js#L67-L75

@rdjuric
Copy link
Contributor Author

rdjuric commented Jun 30, 2021

@Luke9389 @marcaaron It's a good point! We would be restricting too much the use cases of our TEXT fragment.

This commit keeps the containerStyle in our ToolTip (we still don't want the content of our TEXT to go beyond the container), but changes the numberOfLines to be a conditional decided by the isSingleLine prop of our ReportActionItemFragment.

@rdjuric rdjuric requested a review from Luke9389 June 30, 2021 21:22
@Luke9389
Copy link
Contributor

OK, these changes look good @rdjuric 👍

Just want to double-check, did you test the most recent commits on each platform? (I know it's a hassle but regressions have been sprouting up a lot lately).

@rdjuric
Copy link
Contributor Author

rdjuric commented Jun 30, 2021

@Luke9389
Yes! Updated the PR with videos testing the most recent commit on all platforms.

Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks! 👍

@Luke9389 Luke9389 merged commit 27ab3cd into Expensify:main Jul 1, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Jul 1, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@parasharrajat
Copy link
Member

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 1, 2021

Thanks, @parasharrajat. Looking at it now.

@roryabraham
Copy link
Contributor

Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants