fix(v2): Add hooks to detect window resize, toggle off sidebar and navbar in desktop #2932
Conversation
|
Deploy preview for docusaurus-2 ready! Built with commit b189668 |
| function getSize() { | ||
| return isClient | ||
| ? (window.innerWidth > desktopThresholdWidth && desktopSize) || mobileSize | ||
| : undefined; |
There was a problem hiding this comment.
I'd suggest extracting the fn outside of the hook + making it more "readable".
Usage of && and || is hard to follow
function getSize() {
if (isClient) {
return undefined;
}
returrn window.innerWidth > desktopThresholdWidth ? desktopSize : mobileSize;
}
There was a problem hiding this comment.
Indeed, much clearer. I updated the code.
| const windowSize = useWindowSize(); | ||
|
|
||
| useEffect(() => { | ||
| if (windowSize === desktopSize && sidebarShown) { |
There was a problem hiding this comment.
in the same way setWindowSize(getSize()); does not trigger useless re-renders, it's not necessary to test && sidebarShown because React will already noop if setSidebarShown(false); is called while sidebarShown=false.
(also you didn't capture sidebarShown in the deps array)
There was a problem hiding this comment.
There was a problem hiding this comment.
Hum, that's subtle. I updated the code
| return windowSize; | ||
| } | ||
|
|
||
| export {desktopSize, mobileSize}; |
There was a problem hiding this comment.
What about having an object like WindowSizes exposing mobile/desktop attributes.
Similar to this?
There was a problem hiding this comment.
Nice suggestion, I went and did that
packages/docusaurus-theme-classic/src/theme/hooks/useWindowSize.js
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-classic/src/theme/hooks/useWindowSize.js
Outdated
Show resolved
Hide resolved
| const windowSize = useWindowSize(); | ||
|
|
||
| useEffect(() => { | ||
| if (windowSize === windowSizes.desktop && showResponsiveSidebar) { |
There was a problem hiding this comment.
here too you could remove showResponsiveSidebar (or you need to include it in useEffect deps)
| if (windowSize === windowSizes.desktop) { | ||
| setSidebarShown(false); | ||
| } | ||
| }, [windowSize, sidebarShown]); |
There was a problem hiding this comment.
now here sidebarShown is not used in the effect anymore so it's not necessary to add it to the deps :)
There was a problem hiding this comment.
Indeed ! I made the changes
slorber
added
bug
|
thanks :) |
Motivation
Resolve #2750
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)