Feature/onClose prop support #17
Conversation
…Close is triggered in the Dropdown component when the dropdown is no longer in the expanded state
…ed. Prevents from firing onload
|
Damn the package-lock file |
|
Also looks like your Node version in your Netlify build is 2 versions behind. |
harshzalavadiya
left a comment
harshzalavadiya
left a comment
There was a problem hiding this comment.
Hi @mnoster,
Thank you taking time to submit a PR, but can you make suggested changes,
if you can squash all commits to remove multiple comments with interactive rebase that will be nice, otherwise I'll be happy to rebase it for you before merging
| ItemRenderer?: Function; | ||
| selectAllLabel?: string; | ||
| isLoading?: boolean; | ||
| onClose?; |
There was a problem hiding this comment.
Instead of onClose we can give developer freedom to subscribe to both events for example perform some data fetching onOpen and do cleanup onClose so I think it will be better if we can have single simplified prop like onMenuToggle returning boolean value
| children?; | ||
| contentComponent; | ||
| contentProps: object; | ||
| isLoading?: boolean; | ||
| disabled?: boolean; | ||
| shouldToggleOnHover?: boolean; | ||
| labelledBy?: string; | ||
| contentProps: object; | ||
| onClose?; | ||
| contentComponent; | ||
| shouldToggleOnHover?: boolean; |
There was a problem hiding this comment.
There's lot of unnecessary diff here and can be avoided easily can you reorder this as it was and just pass onMenuToggle prop?
| onClose, | ||
| children, | ||
| contentComponent: ContentComponent, | ||
| contentProps, | ||
| isLoading, | ||
| disabled, | ||
| shouldToggleOnHover, | ||
| isLoading, | ||
| labelledBy, | ||
| contentProps, | ||
| contentComponent: ContentComponent, | ||
| shouldToggleOnHover, |
| /* eslint-disable react-hooks/exhaustive-deps */ | ||
| useEffect(() => { | ||
| if (expanded === true) { | ||
| enableOnClose(true); | ||
| } else if (onCloseEnabled && !expanded) { | ||
| onClose(); | ||
| } | ||
| }, [expanded]); | ||
|
|
There was a problem hiding this comment.
We can simplify this logic even more like below this might automatically remove exhaustive-deps
useEffect(() => {
onMenuToggle && onMenuToggle(expanded);
}, [expanded])
|
Closing this PR for a cleaner one: |
2 participants
Closes #16
npm run storybook. Open the dev console and see the 'Closed' log when you expand then close the dropdown.