[No QA] Growl: add action button and loading variant#94865
Conversation
JakubKorytko
left a comment
There was a problem hiding this comment.
nice one but some comments
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
JakubKorytko
left a comment
There was a problem hiding this comment.
second round my man, nothing serious though
JakubKorytko
left a comment
There was a problem hiding this comment.
tbh this reads much cleaner. A few lifecycle and interaction edge cases stood out that are worth sanity-checking before the follow-up PR wires the action growl for real - approving since these are NAB's, good job!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c877a7ed30
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
situchan
left a comment
There was a problem hiding this comment.
NIT: The PR body advertises a loading() indefinite variant, but no loading function is added to src/libs/Growl.ts in this diff.
removed it from the pr body as it's no longer implemented |
|
🤖 Code review Solid, well-commented refactor. Splitting the visible UI into A few things worth addressing before this lands: 1. Native container ignores 2. PR description mentions a 3. Author checklist vs. tests. The checklist has "I added unit tests" checked, but no tests are included and Codecov flags a coverage drop. The nonce-guard / stale-dismissal logic is the subtlest part of this change and is exactly what a small unit test would protect. "N/A – dormant groundwork" is a defensible call, but then that checkbox shouldn't be checked. Minor / non-blocking
I reviewed this at the code level against the PR head; I did not runtime-verify the slide animation on each platform, so please confirm the visual behavior (especially the bottom-right action layout on wide web) during QA. Reply with |
situchan
left a comment
There was a problem hiding this comment.
Above review comments are not blockers but let's make sure to test tablet landscape in implementation PR
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c877a7ed30
ℹ️ 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".
|
|
all comments resolved so it's good to go |
mountiny
left a comment
There was a problem hiding this comment.
Thank you! All comments are NAB
| type GrowlNotificationContentProps = { | ||
| bodyText: string; | ||
| type: GrowlType; | ||
| duration: number; | ||
| action?: GrowlAction; | ||
|
|
||
| /** Identifies this growl instance; passed back through onDismissed so the parent can ignore stale dismissals. */ | ||
| nonce: number; | ||
| onDismissed: (dismissedNonce: number) => void; | ||
| }; |
There was a problem hiding this comment.
lets add docs to all type props
| icon: React.FC<SvgProps> | IconAsset; | ||
| iconColor: string; |
| type GrowlContent = { | ||
| bodyText: string; | ||
| type: GrowlType; | ||
| duration: number; | ||
| action?: GrowlAction; | ||
|
|
| label: string; | ||
| onPress: () => void; |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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/mountiny in version: 9.4.27-1 🚀
|
|
🤖 Help site review: no documentation changes required. This PR adds dormant capability to the in-app Growl notification component (an optional action button, a Because of that:
I did not create a docs draft PR, since there's no customer-facing behavior to document and there's no linked help-site PR to review. When the follow-up PR actually surfaces the "Expense added" growl with a "View" deep link to users, that's the point at which a help-site update may be warranted (if that flow is documented) — feel free to re-run
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.27-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Adds dormant capability to the Growl notification with no behavior change:
show/successaccept an optionalactionbutton and supportingCONST.GROWLdurations, styles, and a reversed theme color. UI is extracted intoGrowlNotificationContentso the shell only mounts content while visible. Groundwork for an upcoming "Expense added" growl with a "View" deep link.Fixed Issues
$ #91408
PROPOSAL:
Tests
N/A - we do not use the growls anywhere else in the app (NO QA)
Offline tests
N/A — this change only modifies the in-app notification component; it adds no API calls or Onyx operations. Existing growls behave identically offline and online.
QA Steps
Same as tests.
// TODO: These must be filled out, or the issue title must include "[No QA]." — this PR is [No QA] (no user-facing change).
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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