feat(Select): introduce variant of Select using FloatingUI [WIP] #12007
Conversation
nicolethoen
requested review from
kmcfaul,
thatblindgeye and
wise-king-sullyman
|
Preview: https://pf-react-pr-12007.surge.sh A11y report: https://pf-react-pr-12007-a11y.surge.sh |
nicolethoen
changed the title
There was a problem hiding this comment.
Some initial file comments below. Overall this looks great, and the best part when changing useFloatingUI to true by default:
One question regarding the future of using FloatingUI: do we plan to rebuild the API to be a little more inline with Floating UI rather than trying to keep the API consistent for now? Seems like there may be additional/alternative properties we could try leveraging from FloatingUI, and just pondering whether our API would work longterm or if it would make for a better consumer experience to clean things up further in a breaking change.
|
|
||
| return ( | ||
| <> | ||
| {trigger && !triggerRef && ( |
There was a problem hiding this comment.
Looks like this line might be what's causing things to not render? Updating to trigger && !!triggerRef renders things again. Issue stems from Select always having triggerRef defined.
| // Configure middleware | ||
| const middleware = useMemo(() => { | ||
| const middlewareArray = [ | ||
| offset(distance), |
There was a problem hiding this comment.
We'd probably want to allow passing a single number or the offset object to allow more custom override, just to try and mimic what was done in https://github.com/patternfly/patternfly-react/pull/11994/files
| onMouseEnter={handleMouseEnter} | ||
| onMouseLeave={handleMouseLeave} | ||
| onFocus={handleFocus} | ||
| onBlur={handleBlur} | ||
| onClick={handleTriggerClick} |
There was a problem hiding this comment.
Not sure if we should defined these and place them on the div here. At least for the onClick, this could result in unexpected firings since the div could be wider than the trigger itself.
| const { refs, floatingStyles, context } = useFloating({ | ||
| placement: floatingUIPlacement, | ||
| middleware, | ||
| open: showPopper, | ||
| whileElementsMounted: autoUpdate | ||
| }); |
There was a problem hiding this comment.
Will we have to pass the onOpenChange option here and tweak how we're handling that in other components? Pressing Escape doesn't close the Select menu when it's opened when using the floating UI.
| const getLanguageDirection = (targetElement: HTMLElement = document.body, defaultDirection: 'ltr' | 'rtl' = 'ltr') => { | ||
| if (!targetElement) { | ||
| return defaultDirection; | ||
| } | ||
| const computedStyle = window.getComputedStyle(targetElement); | ||
| const direction = computedStyle.direction; | ||
| return direction === 'rtl' ? 'rtl' : 'ltr'; | ||
| }; |
There was a problem hiding this comment.
We could import the getLanguageDirection helper from utils instead of redefining it here
| const languageDirection = getLanguageDirection(document.body); | ||
|
|
||
| // Handle position mapping with RTL support | ||
| const internalPosition = useMemo<'left' | 'right' | 'center'>(() => { |
There was a problem hiding this comment.
I might not be passing correct stuff in when testing, or I'm expecting the wrong thing. When I open a Select example ("Select option variants" since it's popper menu is wider than the toggle), by default the popper content extends to the right beyond the toggle. When I toggle RTL to be on, I was expecting the popper menu to then extend to the left beyond the toggle, but it still extends to the right
Default LTR view:
RTL view:
This may not be something we have to address immediately since we currently have issues with this anyway, but just wanted to mention it in case we're expectng this behavior to work differently.
introduce variant of Select using FloatingUI to use optionally instead of Popper
Closes #12000
Closes #12001