83862 tags settings v2#90453
Conversation
|
@mananjadhav 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] |
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 182446c9ed
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
24cfed8 to
e203c87
Compare
|
@mananjadhav This PR is ready for review |
mananjadhav
left a comment
There was a problem hiding this comment.
Left 2 small comments.
| entryScreens: [SCREENS.SETTINGS_TAGS.SETTINGS_TAGS_ROOT], | ||
| getRoute: (orderWeight: number) => `tag-list/${orderWeight}`, | ||
| }, | ||
| SETTINGS_TAG_SETTINGS: { |
There was a problem hiding this comment.
Don't we need the OldRoute mapping?
There was a problem hiding this comment.
@mananjadhav I’m not sure when we should update the route to the old route. In all migration cases where we change the route path, do we always update it to the old route, or are there cases where we actually need to set an oldRoute?
There was a problem hiding this comment.
I think @mjasikowski would be better to answer. My thoughts are if the links are sent over chat then having the migration mapping really helps.
There was a problem hiding this comment.
In general, mappings should be made for all migrated routes that differ from the static ones. While in some cases it might be impossible, it's better to do it in this specific case, as there shouldn't be any mapping issues here 😄
I checked, and this mapping works correctly: /settings/*/tag/*/*': '/settings/$1/tags/tag-settings/$2/$3.
Also, please check if the migrated routes differ from the static ones elsewhere in this PR, and if so, try to add mappings for them too 🙌
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb-settings-tags.mov |
Reviewer Checklist
|
mananjadhav
left a comment
There was a problem hiding this comment.
Completed the checklist with the screencasts updated earlier. One comment here before merging.
Explanation of Change
Fixed Issues
$ #83862
PROPOSAL:
Tests
Same QA step
Offline tests
QA Steps
SETTINGS_TAG_CREATE
Precondition: The workspace has tags enabled, but no tags have been imported or added yet.
https://staging.new.expensify.com:8082/workspaces/74F19F3521623397/tags
to:
https://staging.new.expensify.com:8082/settings/74F19F3521623397/tags
to open the settings flow.
(change
workspaces->settings)SETTINGS_TAGS_EDIT
Single level of tag:
Precondition: The workspace has tags enabled and contains at least one or multiple tags.
https://staging.new.expensify.com:8082/workspaces/74F19F3521623397/tags
to:
https://staging.new.expensify.com:8082/settings/74F19F3521623397/tags
(change
workspaces→settings) to open the settings flow.Multiple level tags:
Precondition: The workspace has tags enabled and contains multiple tags.
https://staging.new.expensify.com:8082/workspaces/74F19F3521623397/tags
to:
https://staging.new.expensify.com:8082/settings/74F19F3521623397/tags
(change
workspaces→settings) to open the settings flow.SETTINGS_TAG_SETTINGS
Precondition: The workspace has tags enabled and contains at least one or multiple tags.
https://staging.new.expensify.com:8082/workspaces/74F19F3521623397/tags
to:
https://staging.new.expensify.com:8082/settings/74F19F3521623397/tags
(change
workspaces→settings) to open the settings flow.SETTINGS_TAG_EDIT
Precondition: The workspace has tags enabled and contains at least one or multiple tags.
https://staging.new.expensify.com:8082/workspaces/74F19F3521623397/tags
to:
https://staging.new.expensify.com:8082/settings/74F19F3521623397/tags
(change
workspaces→settings) to open the settings flow.SETTINGS_TAG_GL_CODE
Precondition: The workspace has tags enabled and contains at least one or multiple tags.
https://staging.new.expensify.com:8082/workspaces/74F19F3521623397/tags
to:
https://staging.new.expensify.com:8082/settings/74F19F3521623397/tags
(change
workspaces→settings) to open the settings flow.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
Screen.Recording.2026-05-05.at.15.41.28.mov