-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Navigation.isActiveRoute function updated to handle currentRoute logic #4382
Navigation.isActiveRoute function updated to handle currentRoute logic #4382
Conversation
…he current route logic
* @param {String} routePath Path to check | ||
* @return {Boolean} is active | ||
*/ | ||
function isActive(routePath) { | ||
function isActiveRoute(routePath) { | ||
const buildLink = useLinkBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from https://reactjs.org/docs/hooks-rules.html#only-call-hooks-from-react-functions. All good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. But it seems that using getPathFromState
will have to also be inside a component only. Any suggestions on this?
React Navigation suggests this approach:
https://reactnavigation.org/docs/screen-tracking/#example
The second approach would then be creating a ref in Navigation
export const currentRouteRef = React.createRef();
and in NavigationRoot.parseAndStoreRoute
currentRouteRef.current = path;
and this quite similar to fetch URL from Onyx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already using navigationRef in the isActiveRoute. but NAB.
heh lint failed now |
Fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving these changes as I believe the use of the hook here is clearer than other over-engineered solutions.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging in version: 1.0.82-8🚀
|
🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀
|
const path = navigationRef.current && navigationRef.current.getCurrentRoute().path | ||
? navigationRef.current.getCurrentRoute().path.substring(1) | ||
const path = navigationRef.current && navigationRef.current.getCurrentRoute().name | ||
? buildLink( | ||
navigationRef.current.getCurrentRoute().name, | ||
navigationRef.current.getCurrentRoute().params, | ||
).substring(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have kept using getCurrentRoute().path
as the first resource and only fallback to buildLink
if needed. This is because the useLinkBuilder
or getPathFromState
does not work well with urls that contain encoded parameters. react-navigation
encodes every param even the encoded ones resulting in double-encoded urls. Those double-encoded urls won't match with the user's urls. - Coming from #33001
@iwiznia Can you please review?
Details
Fixed the current route logic with useLinkBuilder from 'react-navigation'
Fixed Issues
$ Fixes App/4280
Tests
QA Steps
Tested On
Screenshots
Web
autoselect-expensify-card-web.mp4
Mobile Web
autoselect-expensify-card-mweb.mp4
Desktop
autoselect-expensify-card-desktop.mp4
iOS
autoselect-expensify-card-ios.mp4
Android
autoselect-expensify-card-android.mp4