-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Added floating-ui hook usage for ComboBox
#16585
feat: Added floating-ui hook usage for ComboBox
#16585
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need an auto align prop in this case ? Since its a comboBox, auto align should not be an optional behavior like popover. Auto align should be a default behaviour and should not be changed in conbobox.So implementation of floating UI in this case should not be optional( i.e should not depend on auto align prop)
@tay1orjones What do you think on this ?
We can definitely do this if it's not considered a breaking change |
Yeah, I agree with Riddhi on this! It is the same approach I'm having on |
ComboBox
ComboBox
@@ -105,3 +106,5 @@ export const generateItems = (amount, generator) => | |||
.map((_, i) => generator(i)); | |||
|
|||
export const customItemToString = ({ field }) => field; | |||
|
|||
export const waitForPosition = () => act(async () => {}); // Flush microtasks. Position state is ready by this line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to make a note that this comes from https://floating-ui.com/docs/react#testing and more context is here: floating-ui/react-popper#368 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks so good 🔥 just a couple minor things
update comments for interface Co-authored-by: Taylor Jones <[email protected]>
update comment for prop-types Co-authored-by: Taylor Jones <[email protected]>
removing `fallbackAxisSideDirection` and keeping it flip() Co-authored-by: Taylor Jones <[email protected]>
adding placement as we already have direction prop coming in Co-authored-by: Taylor Jones <[email protected]>
This is discussed on slack |
LGTM !! Just need to run yarn format !! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some weird unrelated changes highlighted in percy - might just be due to being out of date from latest with main. I just pushed the button to update, if status checks pass I think this LGTM 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cb30e67
Closes #16470
Added floating-ui for combobox
Changelog
New
useFloating
hook fromfloating-ui
library.autoAlign
which toggles the use of theuseFloating
hook.useEffect
hook to apply styles to theCombobox
usingCSSOM
Changed
ref.setReference
toListBox
andref.setFloating
toListBox.Menu
Testing / Reviewing
Check the new Experimental Auto Align story from the Combobox component.