fix: adjust agent promotional banner UI to match design mocks#91795
Conversation
- Use medium button size on wide layout instead of small - Apply label font size to subtitle text - Reduce gap spacing from gap4 (16px) to gap3 (12px) Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| subtitleStyle={[styles.mt1]} | ||
| style={[styles.borderRadiusComponentLarge, styles.gap4]} | ||
| subtitleStyle={[styles.mt1, styles.textLabel]} | ||
| style={[styles.borderRadiusComponentLarge, styles.gap3]} |
There was a problem hiding this comment.
| style={[styles.borderRadiusComponentLarge, styles.gap3]} | |
| style={[styles.borderRadiusComponentLarge]} |
@MelvinBot No need for gap3 as it is already present in BillingBanner component
There was a problem hiding this comment.
Good catch — removed the redundant gap3. Pushed in 9dee2f2.
gap3 is already applied by the BillingBanner component internally, so specifying it again in the style prop is unnecessary. Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
@dubielzyk-expensify The spacing cannot be reduced too much since we are using flexbox here. Do you have any other feedback here? cc @NicolasBonet here |
|
Mostly just fixing the right part of the promo to use 20px padding from the right and 16px spacing between the X and the button: 
|
|
@MelvinBot Address #91795 (comment) |
Address design review feedback from dubielzyk-expensify: - Use gap4 (16px) between the CTA button and dismiss icon on wide layout - Bundle the button and dismiss icon into a single rightComponent so their spacing is controlled independently of the container gap Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
Addressed the spacing feedback in 111146c:
|
|
cc @Expensify/design for vis (just tweaking it to be like Figma) |
|
@MelvinBot Fix conflicts on this PR |
…weaks Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com> # Conflicts: # src/components/AgentPromotionalBanner.tsx
|
Merge conflicts resolved with The conflict was in the |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
| const dismissIcon = useMemo( | ||
| () => ( | ||
| <PressableWithoutFeedback | ||
| onPress={onDismiss} | ||
| style={[styles.touchableButtonImage]} | ||
| role={CONST.ROLE.BUTTON} | ||
| accessibilityLabel={translate('common.dismiss')} | ||
| sentryLabel={dismissSentryLabel} | ||
| > | ||
| <Icon | ||
| src={expensifyIcons.Close} | ||
| fill={theme.icon} | ||
| /> | ||
| </PressableWithoutFeedback> | ||
| ), | ||
| [onDismiss, styles.touchableButtonImage, translate, dismissSentryLabel, expensifyIcons.Close, theme.icon], | ||
| ); |
There was a problem hiding this comment.
@MelvinBot Change the icon pressable style to [styles.touchableButtonImage, styles.threeDotsMenuIconWidth] in BillingBanner directly and revert this change
There was a problem hiding this comment.
Done in cc42a30.
- Added
styles.threeDotsMenuIconWidthto the pressable inBillingBannerso the constrained icon width lives there. - Reverted the custom
dismissIcon/PressableWithoutFeedbackwiring inAgentPromotionalBannerand went back to passingrightIcon/onRightIconPresstoBillingBanner— it already places the icon next torightComponentwith the correct ordering on narrow vs wide layouts.
…dth in BillingBanner Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
1 similar comment
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
@dubielzyk-expensify Here's the updated banner |
dubielzyk-expensify
left a comment
dubielzyk-expensify
left a comment
There was a problem hiding this comment.
Looks good from a visual perspective. I'll let others handle the code bits :)
|
@MelvinBot Mark all items on Author checklist (no need to check, I have already verified them), and then mark the PR ready for review |
Reviewer Checklist
Screenshots/Videos
|
|
@thelullabyy 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] |
|
🤖 Done, as requested by
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc42a3013f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <PressableWithoutFeedback | ||
| onPress={onRightIconPress} | ||
| style={[styles.touchableButtonImage]} | ||
| style={[styles.touchableButtonImage, styles.threeDotsMenuIconWidth]} |
There was a problem hiding this comment.
When a BillingBanner right icon is pressable (e.g. the agent promo and retry-billing success dismiss buttons), adding threeDotsMenuIconWidth overrides touchableButtonImage's 40px width with componentSizeXSmall (20px at the default font scale). That shrinks the actual tappable area of the close button on touch devices, making the dismiss affordance much harder to hit; adjust the visual spacing without reducing the pressable width, or use hit slop to preserve the old target size.
Useful? React with 👍 / 👎.
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
@shubham1206agra let's fix the checklist please |
|
@NicolasBonet Can you fix the checklist? I can't edit Melvin's PR description |
|
@NicolasBonet looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
I fixed the checklist and it got green once I merged, dunno why it changed but 100% not an emergency. |
|
🚧 @NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.3.97-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. This PR makes purely cosmetic UI adjustments to the
None of these change any feature behavior, navigation, settings, labels, or user workflows — they only affect how the banner looks. I also searched Because no documentation changes are needed, I did not create a draft help site PR. |
|
@joekaufmanexpensify @shubham1206agra Do we have to test anything in this PR? |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.97-1 🚀
|
7 participants
Explanation of Change
Follow-up to #91251 addressing design review feedback on the
AgentPromotionalBannercomponent:textLabel) to the subtitle text instead of the default body sizegap4(16px) togap3(12px) for tighter spacing around the button and dismiss iconFixed Issues
$ #91243
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
No special offline test required — purely cosmetic changes.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari