-
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
Truncated money request preview #30240
Truncated money request preview #30240
Conversation
@situchan All the videos added... Hoping this can be a quick one |
gentle bump @situchan |
Not sure if this is bug or expected. text is scrollable inside preview Screen.Recording.2023-10-26.at.4.37.07.PM.mov |
Wow, interesting. @JmillsExpensify or @trjExpensify do you have any context here? I think there definitely shouldn't be a scrollable area, so not sure how that happened. I would think we just truncate this one after 2-3 lines max, and ideally this component behaves the same as other preview components. |
@situchan I investigated this and apparently, this is an issue in But I have a solution to this: <View style={[styles.flex1, truncatedContainerStyle]}> {/* Change here */}
{!isCurrentUserManager && props.shouldShowPendingConversionMessage && (
<Text style={[styles.textLabel, styles.colorMuted]}>{props.translate('iou.pendingConversionMessage')}</Text>
)}
{shouldShowDescription && <Text style={[styles.colorMuted]}>{description}</Text>}
</View> where const truncatedContainerStyle = StyleUtils.combineStyles([
StyleUtils.getMaximumHeight(3 * variables.fontSizeNormalHeight),
styles.overflowHidden,
]); same can be done for the merchant. |
Agree with Shawn here. Truncate after 2-3 lines (I'd kinda like to see both to see how they feel) with an "..." at the end to let users know there's more once they tap into it. |
@esh-g Did you test that approach on all platforms? |
@situchan Yes, I did test on |
@esh-g what you linked is very old article. can you think of alternative solution, not setting maxHeight? |
@situchan I don't think there is an alternative...
The other option we have is to limit the number of characters in the IOU preview., which can be done easily... |
@esh-g ok, please push changes and update screenshots accordingly with scrolling tested. Make sure to test various text length which fill 1-2 lines, 3 lines, 3+ lines |
Okay @situchan Just a note that there are no ellipsis with this approach, I hope that's fine? |
No, ellipsis should show |
@situchan Then I think it's better we go for a preview with a fixed number of characters instead... because that will be predictable to add the ellipsis and I don't see any downside with that approach either... |
Gentle bump @situchan |
Nope, we should not truncate characters as width / font width will vary depending on devices. |
Sorry I'm late here, I was out for a few days. I don't have context on this one, but agree it shouldn't be scrollable and we should truncate. 👍 |
@situchan I wish there was a solution to that but seeing that even react-native-web was not able to come up with a solution to the scroll problem, I think the variation in the number of lines would be at most 1-2 lines... But still if we want the current approach with the ellipsis, then we would have to add the ellipsis to the next line like in the following image: This is achieved by adding a state var called cc @Expensify/design (to decide with the ellipsis on the next line...) |
I'm not a huge fan of adding a whole line to the preview just for the ellipsis, what do you think @dannymcclain? |
@trjExpensify yeah I'm not a fan of that either. If we can't reliably truncate based on lines I think I'd rather do it based on character count like was suggested here (I think?). I realize that might give us less control over the number of lines that show up but we could probably choose a character count that we're pretty confident wouldn't span to 3 lines. And it would likely look a LOT cleaner. What do you think? |
Yeah, are you suggesting we have an ellipsis still to "signal" the truncation (which I think is important) but after X characters such that it's contained to two lines. I agree that's better than:
|
@trjExpensify Yeah that's exactly what I'm thinking. I also think the ellipsis is pretty important, so trying to find the nicest way to still include it feels like our best option here. |
So do we agree to go ahead with a character limit? If yes, then what is the amount of characters we are thinking? |
@shawnborton Do you mean cutting out the description so that only first 2 lines are visible? We already discussed that and the problem was that there would be no ellipsis in the text.. |
I thought we could use the baked in |
@shawnborton Yes we can use that on native but it is broken for react-native-web. If you have more than one line, it allows the user to scroll on web. This is the reason we decided to cap characters instead of lines.. |
Ah got it, thanks for clarifying! |
Does the change look good? @situchan |
Looks good. Please also test with various width of characters including emojis. |
@situchan Seems like we can fit a bit more emojis than text... but is this something we should change? Because I wouldn't say it is realistic to use only emojis and there is no way to strictly enforce the 2 line limit for different characters... |
bump @situchan |
I will complete review by tomorrow |
bump @situchan |
If that's fine, I will go ahead given that there's no perfect solution to be responsive to all platforms. |
Reviewer Checklist
Screenshots/Videos |
@esh-g I don't see scroll issue in other places of the app where App/src/components/QRShare/index.js Line 58 in d5f5856
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/joelbettner in version: 1.4.22-0 🚀
|
🚀 Deployed to staging by https://github.com/joelbettner in version: 1.4.22-0 🚀
|
It definitely is very strange behaviour for |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.22-6 🚀
|
Details
Fixed Issues
$ #29459
PROPOSAL: #29459 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Screen.Recording.2023-10-19.at.8.46.52.PM-1.mov
Android: mWeb Chrome
Screen.Recording.2023-10-19.at.8.54.37.PM-1.mov
iOS: Native
Screen.Recording.2023-10-24.at.4.45.32.PM.mov
iOS: mWeb Safari
Screen.Recording.2023-10-19.at.9.03.27.PM-1.mov
MacOS: Chrome / Safari
Screen.Recording.2023-10-24.at.4.00.44.PM.mov
MacOS: Desktop
Screen.Recording.2023-10-24.at.4.13.21.PM.mov