-
Notifications
You must be signed in to change notification settings - Fork 4.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
Fixed broken active link in developer docs navbar [Fixes #10813] #13331
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks @0x0OZ! Appreciate you dropping this.
Taking a look here, I think we should probably patch the isHrefActive
function instead, to do this same logic.
My thought here is, that function should answer "is this href active?", which should always be ignoring the hash links (anything after a #
) or any query/search params (anything after a ?
). We shouldn't have to pre-sanitize our pathname when passing to this function in my opinion.
const cleanAsPath = asPath.replace(/#.*/, "");
If you look inside src/lib/utils/url.ts
(where you're importing isHrefActive
, you'll see another helper function called cleanPath
:
// remove any query params or hashes from the path
export const cleanPath = (path: string): string => path.replace(/[$#].+$/, "")
This is very similar to the logic you added, but will also remove any query/search params as well. Let's use this instead.
I would recommend the following:
- Revert these changes to
Link.tsx
- Update the
isHrefActive
function insideurl.ts
: Instead ofpathname === href
, let's wrappathname
withcleanPath
, ie:cleanPath(pathname) === href
- We're using
cleanPath
insideisHrefActive
so let's move thecleanPath
function above the other so we're not using functions before they're declared. This isn't strictly necessary due to JS hoisting but is good practice. - Make sure we run the linter on this to keep it tidy:
yarn prettier --write src/lib/utils/url.ts
- Run
yarn dev
and test it out in the browser to make sure this is working as expected
Hope that helps!
I think this looks good |
This issue is stale because it has been open 30 days with no activity. |
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.
🎉 Beautiful! Sorry for delay, looks great @0x0OZ! Bringing in
Description
There was an issue checking
isHrefActive
inLink.tsx
because of the unremoved hashtags from the URL.I don't know if that's the best way to deal with it or if this fix breaks assumptions on other components that assume hashtags remain in that check.
I am not really a good developer, so it will be great if someone double checks (patch source)
Related Issue
Fixes #10813