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.

Sign up for GitHub

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
merged 4 commits into from
Aug 3, 2021
Merged

Navigation.isActiveRoute function updated to handle currentRoute logic #4382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7fb86db
fix(active-route): Navigation.isActiveRoute function updated to add t…
mananjadhav Aug 2, 2021
b7658db
chore(is-active-route): Updated implementation comment
mananjadhav Aug 3, 2021
f26b5a0
style(lint-fixes): Split commment for lint max-len
mananjadhav Aug 3, 2021
e43b311
style(lint-fixes): Split commment for lint max-len
mananjadhav Aug 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/libs/Navigation/Navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import _ from 'underscore';
import React from 'react';
import {StackActions, DrawerActions} from '@react-navigation/native';
import {StackActions, DrawerActions, useLinkBuilder} from '@react-navigation/native';
import PropTypes from 'prop-types';
import Onyx from 'react-native-onyx';
import linkTo from './linkTo';
Expand Down Expand Up @@ -125,13 +125,22 @@ function dismissModal(shouldOpenDrawer = false) {
/**
* Check whether the passed route is currently Active or not.
*
* Previous implementation used navigationRef.current.getCurrentRoute().path, which was
* undefined in the first navigation. Hence, in our current solution, we're rebuilding
* the active route using the useLinkBuilder hook that takes routeName and params as the arguments.
*
* @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.

Apart from https://reactjs.org/docs/hooks-rules.html#only-call-hooks-from-react-functions. All good.

Copy link
Collaborator Author

@mananjadhav mananjadhav Aug 3, 2021 •
edited
Loading

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 •
edited
Loading

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.


// We remove First forward slash from the URL before matching
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)
Comment on lines -133 to +142
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

: '';
return path === routePath;
}
Expand Down Expand Up @@ -167,7 +176,7 @@ DismissModal.defaultProps = {
export default {
navigate,
dismissModal,
isActive,
isActiveRoute,
goBack,
DismissModal,
closeDrawer,
Expand Down
4 changes: 2 additions & 2 deletions src/pages/workspace/WorkspaceSidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ const WorkspaceSidebar = ({translate, isSmallScreenWidth, policy}) => {
action: () => {
Navigation.navigate(ROUTES.getWorkspaceCardRoute(policy.id));
},
isActive: Navigation.isActive(ROUTES.getWorkspaceCardRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspaceCardRoute(policy.id)),
},
{
translationKey: 'common.people',
icon: Users,
action: () => {
Navigation.navigate(ROUTES.getWorkspacePeopleRoute(policy.id));
},
isActive: Navigation.isActive(ROUTES.getWorkspacePeopleRoute(policy.id)),
isActive: Navigation.isActiveRoute(ROUTES.getWorkspacePeopleRoute(policy.id)),
},
];

Expand Down