-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
There was a problem hiding this 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?
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 |
@Luke9389 @marcaaron It's a good point! We would be restricting too much the use cases of our This commit keeps the |
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). |
@Luke9389 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks! 👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Just found an regression https://expensify.slack.com/archives/C01GTK53T8Q/p1625174167354500 |
Thanks, @parasharrajat. Looking at it now. |
Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday. |
Details
Fixes an issue where our display name wasn't respecting the size of it's container.
Fixed Issues
#3172
Tests
QA Steps
Tested On
Screenshots
Web
Web.mov
Mobile Web
mWeb.mp4
Desktop
desktop.mov
iOS
iOS.mp4
Android
Android.mp4