Skip to content
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

Merged

Conversation

mananjadhav
Copy link
Collaborator

@mananjadhav mananjadhav commented Aug 2, 2021

@iwiznia Can you please review?

Details

Fixed the current route logic with useLinkBuilder from 'react-navigation'

Fixed Issues

$ Fixes App/4280

Tests

  1. Visited routes from Create workspace and visit workspace links to see if the auto-select is visible
  2. Reload. of the URLs done on the Web platform to see if the selection is retained
  3. Switched menus to see if existing functionality is intact

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

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

@mananjadhav mananjadhav requested a review from a team as a code owner August 2, 2021 20:12
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team August 2, 2021 20:12
src/libs/Navigation/Navigation.js Outdated Show resolved Hide resolved
* @param {String} routePath Path to check
* @return {Boolean} is active
*/
function isActive(routePath) {
function isActiveRoute(routePath) {
const buildLink = useLinkBuilder();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@mananjadhav mananjadhav Aug 3, 2021

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

Copy link
Member

@parasharrajat parasharrajat Aug 3, 2021

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.

iwiznia
iwiznia previously approved these changes Aug 3, 2021
@iwiznia
Copy link
Contributor

iwiznia commented Aug 3, 2021

heh lint failed now

@mananjadhav
Copy link
Collaborator Author

Fixed

Copy link
Contributor

@luacmartins luacmartins left a 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.

@luacmartins luacmartins merged commit 829fd76 into Expensify:main Aug 3, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Aug 3, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Comment on lines -133 to +142
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)
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-select the Expensify Card tab when first creating a workspace
6 participants