Refactor Dropdown to use functional component #23142
Conversation
sc81
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
ZebulanStanphill
added
[Type] Code Quality
| const [ isOpen, setIsOpen ] = useState( false ); | ||
| const isFirstRender = useRef( true ); | ||
|
|
||
| this.toggle = this.toggle.bind( this ); | ||
| this.close = this.close.bind( this ); | ||
| this.closeIfFocusOutside = this.closeIfFocusOutside.bind( this ); | ||
|
|
||
| this.containerRef = createRef(); | ||
|
|
||
| this.state = { | ||
| isOpen: false, | ||
| }; | ||
| } | ||
|
|
||
| componentWillUnmount() { | ||
| const { isOpen } = this.state; | ||
| const { onToggle } = this.props; | ||
| if ( isOpen && onToggle ) { | ||
| onToggle( false ); | ||
| } | ||
| } | ||
|
|
||
| componentDidUpdate( prevProps, prevState ) { | ||
| const { isOpen } = this.state; | ||
| const { onToggle } = this.props; | ||
| if ( prevState.isOpen !== isOpen && onToggle ) { | ||
| useEffect( () => { | ||
| if ( isFirstRender.current ) { | ||
| isFirstRender.current = false; | ||
| } else if ( onToggle ) { | ||
| onToggle( isOpen ); | ||
| } | ||
| } | ||
| return () => { | ||
| if ( isOpen && onToggle ) { | ||
| onToggle( false ); | ||
| } | ||
| }; | ||
| }, [ isFirstRender, isOpen, onToggle ] ); |
There was a problem hiding this comment.
I might be missing an implementation detail, but I think the goal here is to trigger onToggle when the state changes, and that could also be handled now as a custom hook, something like the following untested pseudocode:
// Custom hook.
function useObservableState( initialState, onStateChange ) {
const [ value, setValue ] = useState( initialState );
return [ value, ( newValue ) => {
setValue( newValue );
onStateChange( newValue );
} ];
}
// ...
// Usage in the component.
const [ isOpen, setIsOpen ] = useObservableState( false, onToggle );(I would put this hook function above the component in the same file)
Would still also need some separate handling for unmounting, but that then becomes simpler:
// Fire `onToggle` when the component unmounts.
useEffect( () => () => onToggle( false ) );There was a problem hiding this comment.
Yes. This should work. I'll check it later.
There was a problem hiding this comment.
Thanks for your contribution @sc81. It's great that the docs and tests were also updated as part of this.
I notice that tests are failing at the moment. I think it's worth trying to rebase the PR against the master branch and pushing again, as I think some code that fixes broken tests was merged to master.
* Refactor tests * Refactor DropdownMenu tests to get rid of weird errors * Add documentation for onToggle and onClose properties * Add useObservableState
|
@talldan Should we merge this? |
|
Yep, restarted the tests and they look good now 👍 Thanks again for all your work here @sc81. |
|
No problem. And thank you @talldan |
Refactor tests
Refactor DropdownMenu tests to get rid of weird errors
Add documentation for onToggle and onClose properties
Description
How has this been tested?
Screenshots
Types of changes
Checklist: