Replace FullscreenMode from Class components to functional components. #32925
Conversation
github-actions
bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @chiilog! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
| return null; | ||
| } | ||
| } | ||
| }, [ isActive ] ); |
There was a problem hiding this comment.
I think there are some issues with the logic here, I see:
- isSticky is not a state in the previous implementation and changing its value shouldn't cause an effect to run, basically it needs to be stored in a ref (
useRef) - We can probably combine the effects that have
isActiveas a dependency
There was a problem hiding this comment.
@youknowriad Thank you for the review. I modified useStatus to useRef.
| useEffect( () => { | ||
| return () => { | ||
| if ( isSticky ) { | ||
| if ( isSticky.current ) { | ||
| document.body.classList.add( 'sticky-menu' ); | ||
| } | ||
| }; | ||
| }, [ isSticky ] ); |
There was a problem hiding this comment.
the dependency sound useless
There was a problem hiding this comment.
@youknowriad Oops! I overlooked the specification of useRef. I fixed it again.
| // `is-fullscreen-mode` is set in PHP as a body class by Gutenberg, and this causes | ||
| // `sticky-menu` to be applied by WordPress and prevents the admin menu being scrolled | ||
| // even if `is-fullscreen-mode` is then removed. Let's remove `sticky-menu` here as | ||
| // a consequence of the FullscreenMode setup | ||
| if ( document.body.classList.contains( 'sticky-menu' ) ) { | ||
| this.isSticky = true; | ||
| isSticky.current = true; |
There was a problem hiding this comment.
Thanks for the update, so if I'm reading properly, isSticky is only used inside that effect, it means it can be just a local variable right?
There was a problem hiding this comment.
Do you mind making that change?
There was a problem hiding this comment.
That makes sense. I've integrated a useEffect that does cleanup, so isSticky no longer needs a useRef and can be defined in a let.
Related #22890
Replace FullscreenMode from Class components to functional components.
I changed test code because
useEffectdoes not seem to be executed when the component is rendered withshallow.How has this been tested?
npm run test-unitand
Checklist:
*.native.jsfiles for terms that need renaming or removal).