Update "Hold expense" modal copy and icons #53354
Conversation
gijoe0295
changed the title
melvin-bot
bot
requested review from
parasharrajat
and removed request for
a team
|
@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
On mobile, the bottom corners should not be rounded: |
| @@ -170,9 +207,11 @@ function FeatureTrainingModal({ | |||
| User.dismissTrackTrainingModal(); | |||
| } | |||
| setIsModalVisible(false); | |||
| Navigation.goBack(); | |||
| if (onboardingIsMediumOrLargerScreenWidth) { | |||
| Navigation.goBack(); | |||
There was a problem hiding this comment.
We only need to call goBack on large screens because the hold request modal is rendered inside a dedicated page route. On small screens, it's just a modal.
There was a problem hiding this comment.
This is creating an issue. For Track expense feature training we show the route on all platforms so we need to go back on all of them.
There was a problem hiding this comment.
Yeah I think we no longer need modal for small screens because we stopped using RHP for hold modal.
|
For light mode, I wonder if we should make the "hold" text use a lighter color? My gut says yes, but let's see what @Expensify/design thinks too.
I would just opt to use the same color we use for button text. |
| deleteHoldTitle: 'Eliminar lo que no se pagará', | ||
| deleteHoldExplain: 'En el raro caso de que algo se bloquee y no se pague, la persona que solicita el pago debe eliminarlo.', | ||
| holdEducationalTitle: 'Esta solicitud está', | ||
| holdEducationalText: 'retenida', |
There was a problem hiding this comment.
retenida is only used in hold request modal. Hold in other places remains bloquear. Context: #52655 (comment)
Updated this. |
|
@gijoe0295 Thanks for waiting. Can you please resolve the conflicts? |
|
Not done yet. Need to retest after resolving the conflicts. |
|
@gijoe0295 Any update? |
|
Is there a difference in the space between the headline and description on mobile and desktop? It almost feels like it's bigger on desktop? Otherwise the modal itself looks good to me 👍 |
|
Good catch Jon - I think we use a standard gap of like 8px or something between headlines and the paragraph that immediately follows? |
|
I'm seeing both |
|
Same, I think that would be my vote too. |
|
@gijoe0295 Can you please confirm the margin used and make sure it is 8px as requested here #53354 (comment) #53354 (comment)? |
|
@shawnborton @dannymcclain I've made the space between title & description consistently
Do these look fine to you? |
|
That feels good to me. Just noting that because of the background color of the "Hold" badge, the gap between headline and text will feel a bit larger than what we're used to seeing elsewhere. But I can get down with what you have above. |
|
@shawnborton Just noting that the mobile still have bigger margin. Only desktop and web has 8p gap. Do you agree with this or we should use same? |
|
Yeah, I would think it's the same everywhere. I would imagine the design team will agree too :) |
Yup! |
| @@ -41,32 +32,33 @@ function ProcessMoneyRequestHoldMenu({isVisible, onClose, onConfirm, anchorPosit | |||
| return unsub; | |||
| }, [navigation, onClose]); | |||
|
|
|||
| const title = useMemo( | |||
| () => ( | |||
| <View style={[styles.flexRow, styles.alignItemsCenter, onboardingIsMediumOrLargerScreenWidth ? styles.mb1 : styles.mb2]}> | |||
There was a problem hiding this comment.
this is supposed to be same on all platforms 8px.
There was a problem hiding this comment.
We already have a 4px gap on desktop in FeatureTrainingModal:
App/src/components/FeatureTrainingModal.tsx
Line 261 in 9be0b9f
|
I too agree with the designers |
|
@gijoe0295 looks like your checklist is missing the Android native screenshots, can you provide those please? @parasharrajat please make sure all screenshots are updated in the reviewer checklist before we merge. |
There was a problem hiding this comment.
Wow this was a big one, thanks @gijoe0295 and @parasharrajat. The code LGTM! Let's just get the correct screenshots in place and I'll merge it.
|
Oops sorry that was my bad, I had trouble building Android and forgot to retry later. I'll do it now. |
|
@MarioExpensify I updated Android video. |
|
✋ 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/MarioExpensify in version: 9.0.87-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.0.87-3 🚀
|
Explanation of Change
Fixed Issues
$ #52655
PROPOSAL: #52655 (comment)
Tests
Offline tests
NA
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2025-01-16.at.00.39.47.mov
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
Screen.Recording.2024-12-03.at.02.26.16-recording.mov
MacOS: Chrome / Safari
MacOS: Desktop