-
Notifications
You must be signed in to change notification settings - Fork 174
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
Fix the relative path bug #261
Fix the relative path bug #261
Conversation
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.
LGTM
return window.location.hash + '/' + path; | ||
} else { | ||
return window.location.pathname + '/' + path; | ||
export const buildFromRelativePath = node => { |
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.
I think it is safer to align the path by number of path segments. My suggestion would be:
export const buildFromRelativePath = node => {
let windowPath = LuigiConfig.getConfigValue('routing.useHashRouting')
? getPathWithoutHash(window.location.hash)
: window.location.pathname;
if (node.parent && node.parent.pathSegment) {
const nodePathSegments = trimLeadingSlash(getNodePath(node.parent)).split('/');
const windowPathSegments = trimLeadingSlash(windowPath).split('/');
if (windowPathSegments.length > nodePathSegments.length) {
windowPath = windowPathSegments.slice(0, nodePathSegments.length).join('/');
}
}
return addLeadingSlash(concatenatePath(windowPath, node.link));
};
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.
A small suggestion here, for consistency and avoiding issues with leading / trailing slashes in the future or between routing strategies, I would use helper methods to normalize nodePathSegments
and windowPathSegments
before applying split, something like:
const nodePathSegments = Helpers.normalizePath(getNodePath(node.parent)).split('/');
const windowPathSegments = Helpers.normalizePath(windowPath).split('/');
Also what is being checked in if (windowPathSegments.length > nodePathSegments.length - 1)
is at least for me not straightforward to understand, maybe a variable with a meaningful name would be helpful.
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.
Yeah, I removed the '-1', it was not necessary with properly prepared paths.
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.
I applied suggested changes :)
The only difference is that instead of
windowPath = windowPathSegments.slice(0, nodePathSegments.length).join('/');
I used
windowPath = nodePathSegments.join('/');
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.
I would rather stick to windowPath = windowPathSegments.slice(0, nodePathSegments.length).join('/');
because of dynamic path segments. It might work now but in the future after a planned refactoring (because of mutating the navigation data structure that we do not want to happen) it will get broken for sure.
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.
You are right, I reverted my latest commit.
* Fix the relative path bug
Description
Changes proposed in this pull request:
Related issue(s)