feat: Expand GBR/RBR with an "action badge"#85025
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
![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: e7b3cd6738
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d4f8c3eee
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d4f8c3eee
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed08930183
ℹ️ 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".
melvin-bot
bot
requested review from
ikevin127 and
trjExpensify
and removed request for
a team
|
@ikevin127 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f3129f1b5
ℹ️ 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".
|
🚧 @shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Feels pretty good. Can you adjust the small condensed badge (at the global leve) to make the icon just 4px away from the text? Right now the gap feels a bit too big: It's currently 8px, it should be 4px: |
|
When an active item has a default badge, we need to adjust the badge's background color: Take a look at Workspace > Invoices for instance: |
@shawnborton should we update badge background on active item only or both active and inactive to match workspace tab? EDIT: nvm, I see only active item on workspace tab. |
|
In production we don't have this feature, right? The badges now have text in them, so we should improve the accessibility labels here imo. |
ok what is new announcement you'd suggest? Add action at the end of "Your review is required"? i.e. "You review is required. Approve" |
|
@aimane-chnaif yes i think appending using a comma is the way to go
And for pinned, just Pinned would work |
|
🚧 @shawnborton 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! 🧪🧪
|
|
@shawnborton can you plz check comment to confirm |
|
The updated colors look good to me at least 👍 |
|
All comments are addressed |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #84764 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@codex review |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks for testing, I think I would just keep an eye on this PR and if it did not cause any performance regression, but the changes were tested and look good, we can move ahead
| if (hasKeyTriggeredCompute(ONYXKEYS.COLLECTION.POLICY, sourceValues)) { | ||
| if (isFullyComputed && Object.keys(previousPolicies ?? {}).length === 0 && Object.keys(policies ?? {}).length > 0) { | ||
| isFullyComputed = false; | ||
| } | ||
| previousPolicies = policies; | ||
| } | ||
|
|
There was a problem hiding this comment.
@TMisiukiewicz @BartekObudzinski I would keep an eye on this from performance perspective
There was a problem hiding this comment.
💡 Codex Review
Line 88 in 85516e7
Keep Badge default wrapper spacing unchanged
Changing the shared Badge base margin from ml1 to ml2 applies to every badge instance, not just the new LHN action badges. This introduces broad spacing regressions in existing screens that rely on the default badge layout (or only partially override it), so the LHN spacing should be handled locally instead of modifying the global default.
ℹ️ 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".
|
@aimane-chnaif the badge margin, that has also been accepted by design team right? |
right |
|
🚧 @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.3.37-0 🚀
|
|
Deploy Blocker #85187 was identified to be related to this PR. |
|
Deploy Blocker #85201 was identified to be related to this PR. |
9 participants
Explanation of Change
Based on discussion:
Note: I intentionally disabled this feature in production yet to avoid PR being reverted. This gate will be removed once this passes QA
Fixed Issues
$ #84764
Tests
Submit,Approve,Pay) shows properly (previously 🟢)Fixshows (previously 🔴)Pinnedwith small pin icon (previously large pin icon only)Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.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