refactor(primary-action-dropdown): use hooks #965
Conversation
| this.setState({ isOpen: false }); | ||
| }; | ||
| React.useEffect(() => { | ||
| window.addEventListener('click', handleGlobalClick); |
There was a problem hiding this comment.
this replaces the componentDidMount and componentWillUnmount
|
|
||
| act(() => { |
There was a problem hiding this comment.
needed this or else got warnings in the test
There was a problem hiding this comment.
Since RTL already includes this, can you maybe try to click on the rendered container?
fireEvent.click(rendered.container)There should be no need for using act directly
src/components/dropdowns/primary-action-dropdown/primary-action-dropdown.js
Show resolved
Hide resolved
| isOpen: false, | ||
| }; | ||
| const PrimaryActionDropdown = props => { | ||
| const ref = React.useRef(); |
There was a problem hiding this comment.
Can we name the hook better?
|
|
||
| act(() => { |
| @@ -230,7 +230,7 @@ const PrimaryActionDropdown = props => { | |||
| return () => { | |||
| window.removeEventListener('click', handleGlobalClick); | |||
| }; | |||
| }, [handleGlobalClick]); | |||
| }); | |||
There was a problem hiding this comment.
We want it to run once not on each render right. So empty array?
| setIsOpen(false); | ||
| } | ||
| }, | ||
| [ref] |
There was a problem hiding this comment.
I think I saw an issue or something related to defining DOM elements (refs) as dependencies. I’ll see if I can find more about it but maybe we should double check if it makes sense to do so.
src/components/dropdowns/primary-action-dropdown/primary-action-dropdown.js
Show resolved
Hide resolved
src/components/dropdowns/primary-action-dropdown/primary-action-dropdown.js
Show resolved
Hide resolved
| <DropdownHead | ||
| iconLeft={primaryOption.props.iconLeft} | ||
| isDisabled={primaryOption.props.isDisabled} | ||
| onClick={isOpen ? () => setIsOpen(false) : primaryOption.props.onClick} |
| chevron={ | ||
| <DropdownChevron | ||
| ref={ref} | ||
| onClick={ |
|
|
||
| act(() => { |
There was a problem hiding this comment.
Since RTL already includes this, can you maybe try to click on the rendered container?
fireEvent.click(rendered.container)There should be no need for using act directly
emmenko
force-pushed
the
ml-primary-action-hook
branch
from
3872bcf to
b83d74f
Compare
emmenko
force-pushed
the
ml-primary-action-hook
branch
from
b83d74f to
5dfdb54
Compare
| componentDidMount() { | ||
| window.addEventListener('click', this.handleGlobalClick); | ||
| } | ||
| const { onClick } = primaryOption.props; |
src/index.js
Outdated
| @@ -141,3 +140,5 @@ export { default as version } from './version'; | |||
| export { | |||
| default as ContentNotification, | |||
| } from './components/notifications/content-notification'; | |||
|
|
|||
| export * from './hooks'; | |||
There was a problem hiding this comment.
Does UIKit really want to export it's hooks already? This would maybe even a feature releasea as a result.
There was a problem hiding this comment.
It was already exposed actually, I just refactored it to export * instead of export { default as useToggleState }
There was a problem hiding this comment.
Hm. I, personal opinion ahead, liked it before more then. The library opts into what it wants to expose instead of defaulting to everything.
There was a problem hiding this comment.
Sure my bad, I'll revert this change then
Summary
Refactors the component
PrimaryActionDropdownto use hooks