Conversation
melvin-bot
Bot
requested review from
huult and
trjExpensify
and removed request for
a team
![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: 698c0258a7
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
backTo project PR 👍
|
@mananjadhav Could you please review the Codex comment? |
| <HeaderWithBackButton | ||
| title={translate('task.task')} | ||
| onBackButtonPress={() => Navigation.goBack(route.params.backTo)} | ||
| onBackButtonPress={() => Navigation.goBack(backPath === ROUTES.HOME ? ROUTES.REPORT_WITH_ID.getRoute(report?.reportID) : backPath)} |
There was a problem hiding this comment.
Sorry, I don’t see a case where backPath is home, since we don’t have a route like https://dev.new.expensify.com:8082/home/description. Could you share a case where this happens? thank you!
There was a problem hiding this comment.
It's more of a fallback because useDynamicBackPath returns ROUTE.HOME. In that scenarios we're using fetching the report route..
|
What about |
…sk-title-description
…sk-title-description
I'll add as a follow up. |
…sk-title-description
mananjadhav
changed the title
TASK_TITLE, REPORT_DESCRIPTIONTasks and Notes
| path: 'notes', | ||
| entryScreens: [SCREENS.REPORT, SCREENS.REPORT_DETAILS.ROOT, SCREENS.PROFILE_ROOT], | ||
| }, | ||
| PRIVATE_NOTES_EDIT: { |
There was a problem hiding this comment.
@collectioneur For this one too, what would be the ideal way to handle these kind of routes? We have our PRIVATE_NOTES_LIST which has /notes in the URL. But when we edit it appends `notes/:accountID/edit.
For now it appends notes/notes/:accountID/edit.
I could remove notes from the prefix but what if page opens from other entry screens apart from /notes listing.
There was a problem hiding this comment.
Okay, here is how we can solve this. The main issue here is the notes/:reportID/edit page.
To fix this, you need to take the base component of this page and create two separate wrapper pages around it, each with its own navigation logic. These two new screens should be attached to two distinct dynamic routes:
- The first route (for example
edit-notes/:accountID) will handle navigation from everywhere except the dynamic/notesscreen. - The second route (e.g.,
:accountID/edit) will be used only when navigating specifically from/notes.
It is crucial to make sure the route paths don't overlap or conflict. You want to avoid a situation where one path mimics the combination of the others (for example, having /notes/:accountID/edit as the first route, and /notes + :accountID/edit as the second). If they are set up that way, the router will always pick the first dynamic screen on a page refresh because its route pattern is longer/more specific.
Each wrapper will have slightly different "go back" behavior:
- The first route will skip
/notesand return the user straight to their previous page. - The second route will return the user specifically to the
/notespage.
I hope I explained this clearly 😄
There was a problem hiding this comment.
Checking this.
4 participants
Explanation of Change
Fixed Issues
$ #83371
PROPOSAL:
Tests
Task title (r/:reportID/title)
Report / task description (r/:reportID/description)
Observe the URL ending with /description after the report segment (e.g. r//description).
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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))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-bt-task-title-desc.mov
Android: mWeb Chrome
mweb-chrome-bt-task-title-desc.mov
iOS: Native
ios-bt-task-title-desc.mov
iOS: mWeb Safari
mweb-safari-bt-task-title-desc.mov
MacOS: Chrome / Safari
web-bt-task-title-desc.mov