-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix: Misaligned Arrow Icons in Trip Summary #61901
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
base: main
Are you sure you want to change the base?
Conversation
@shawnborton @dannymcclain @dubielzyk-expensify , can you please approve this preview : Flight: Train: If you want to test more maybe you can test on adhoc build ? let me know if any changes are required |
I like this idea, but how will we keep it nice looking when one name is short and the other is long? For example:
Just trying to figure out how we keep the arrows aligned when the content doesn't super match in length 🤔 |
Yeah, fair point. My thinking is that we should just have a max-width of 50% so that if we do need to truncate, we get even spacing. But I am definitely open to other ideas, as this is a tricky one. |
Let me know which decision we land on, will update the code accordingly, should i update to max-width 50% and let you guys test it? |
I'd be down with that, yeah. And then don't force the width though so we get the inline style we currently have. |
I'd be keen to see how this looks tbh. It's definitely a tricky one, but I think whats showing above when the location string is short looks a bit off. |
@shawnborton @dannymcclain @dubielzyk-expensify Is it okay now?: Maximum width of startName: Medium width of startName: Small startName but long endName: Flight: |
I think it would be better if you can trigger ad-hoc build and test this one as it is a little tricky one |
I think what you have above looks better... again, recognizing that this one is tricky and we don't have a great solution for now. I think in the future, we might re-style these trip preview cards to be more akin to a report preview, but I think this works for now. In the meantime, I will run a new test build for us. |
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@shawnborton @dannymcclain @dubielzyk-expensify can any of you test it with real data and confirm this one please |
Explanation of Change
Fixed Issues
$ #61740
PROPOSAL: #61740 (comment)
Tests
Same as QA step
Offline tests
Same as QA step
QA Steps
Verify that Arrows should be vertically and horizontally aligned and at the center of the preview
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop