fix(v2): navbar dropdown crash when item.to is undefined #3662
Conversation
|
It's not a very semantical fix as the path shouldn't be checked at all in case there're items on the Navbar item, but apparently, normalization wouldn't be required in this case, and fixing with check |
|
Deploy preview for docusaurus-2 ready! Built with commit 1724fee |
| @@ -8,5 +8,5 @@ | |||
| // Compare the 2 paths, ignoring trailing / | |||
| export const isSamePath = (path1, path2) => { | |||
| const normalize = (str) => (str.endsWith('/') ? str : `${str}/`); | |||
| return normalize(path1) === normalize(path2); | |||
| return path1 && path2 ? normalize(path1) === normalize(path2) : true; | |||
There was a problem hiding this comment.
thanks
What if one path is undefined and the other is defined, wouldn't it wrongly return true?
There was a problem hiding this comment.
This function only makes sense if a path exists as a field, otherwise it doesn't matter what it returns. If path is missing this function shouldn't be called at all, which happens when Navbar has a drop-down which holds a list of entries instead of a path.
With this quick fix I made it check for undefined and return arbitrary value further to hope the underlying logic works and with "true" it does work, probably with false as well as there's just no path.
The real fix should be somewhere in the navbar generation and there's obviously no test for a navbar with a drop-down which would catch this.
In 2.0.0-alpha.65 it still works fine. With .66 it breaks. Removing a drop-down from docusaurus config makes it work with with .66 as well.
The fix I propose is doing nothing bad, it simply does the check only if none of the values are undefined and returns smth to avoid 'undefined' further.
There was a problem hiding this comment.
It's more of a workaround. I'll see if I can fix it in the reverting of the Navbar
| @@ -159,7 +159,7 @@ function NavItemMobile({ | |||
| const menuListRef = useRef<HTMLUListElement>(null); | |||
| const {pathname} = useLocation(); | |||
| const [collapsed, setCollapsed] = useState( | |||
| () => !items?.some((item) => isSamePath(item.to, pathname)) ?? true, | |||
| () => !items?.filter((item) => item.to).some((item) => isSamePath(item.to, pathname)) ?? true, | |||
There was a problem hiding this comment.
This now filters out all the navbar links with href instead of to so that we don't get undefined down the line. Path normalization is not required on external links I assume? Or instead of passing further item.to, you have to path just item and later check the path for to and href
| @@ -7,6 +7,6 @@ | |||
|
|
|||
| // Compare the 2 paths, ignoring trailing / | |||
| export const isSamePath = (path1, path2) => { | |||
| const normalize = (str) => (str.endsWith('/') ? str : `${str}/`); | |||
| return normalize(path1) === normalize(path2); | |||
| const normalize = (str) => (str?.endsWith('/') ? str : `${str}/`); | |||
There was a problem hiding this comment.
This is another safeguard for undefined. With such a fix it will work even without filtering done above and lead to a consistent Navbar menu structure.
slorber
changed the title
|
Thanks. I've updated your PR, as I think making isSamePath support undefined will be better for the future, as we'll likely need this function in other themes we implement as well |
|
Looks good, thanks for acting on this. You're doing a great job guys:) |
|
thanks :) |
Motivation
According to the docs, you can create a dropdown in the Navbar. With alpha.66 this fails because there's a path normalizer, which doesn't handle undefined if Navbar item is dropdown instead of a link which has a path.
Have you read the Contributing Guidelines on pull requests?
Yes and already contributed once.
Test Plan
I've written a test:
Before my fix:
After my fix:
Related PRs
This fix actually makes docs relevant as currently, dropdown wouldn't work.