Use popper #299
Use popper #299
Conversation
src/DropdownMenu.js
Outdated
| import DropdownContext from './DropdownContext'; | ||
|
|
||
| export function useDropdownMenu(options = {}) { | ||
| // const prevProps = usePrevious(props); |
There was a problem hiding this comment.
?
i guess this is all in usePopper now?
There was a problem hiding this comment.
yeah I think it's unneeded here now, but i need to try in RB to confirm
src/Dropdown.js
Outdated
| import PropTypes from 'prop-types'; | ||
| import uncontrollable from 'uncontrollable'; | ||
| import useUncontrolled from 'uncontrollable/hook'; |
|
Hi @jquense do you need some help with this? |
|
happy to get some help if you wanna jump in I'm just a bit short on time at the moment |
This comment has been minimized.
This comment has been minimized.
src/Dropdown.js
Outdated
|
|
||
| if (show && !prevOpen) { | ||
| this.maybeFocusFirst(); | ||
| if (focusStype == null) { |
There was a problem hiding this comment.
Does it make sense to useMemo this outside of this callback? I don't think this needs to be calculated on-the-fly.
There was a problem hiding this comment.
in practice it only runs on show so it's probably fine
| focusInDropdown.current = false; | ||
| focusToggle(); | ||
| } | ||
| // only `show` should be changing |
There was a problem hiding this comment.
Does it make sense to guard and check the previous value of show anyway?
| const handleClose = e => { | ||
| if (!context.toggle) return; | ||
|
|
||
| context.toggle(false, e); |
There was a problem hiding this comment.
| context.toggle(false, e); | |
| context.toggle?.(false, e); |
There was a problem hiding this comment.
Optional chaining plugin is not specified in babel config
| } | ||
|
|
||
| const handleClose = e => { | ||
| if (!context.toggle) return; |
There was a problem hiding this comment.
| if (!context.toggle) return; |
Co-Authored-By: Jimmy Jia <[email protected]>
|
Is this ready to merge guys? |
I think done