-
Notifications
You must be signed in to change notification settings - Fork 561
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
Fix selection for controlled combobox #865
Fix selection for controlled combobox #865
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 43e4d0c:
|
packages/combobox/src/index.tsx
Outdated
@@ -312,6 +312,8 @@ export const Combobox = React.forwardRef(function Combobox( | |||
let id = useId(props.id); | |||
let listboxId = id ? makeId("listbox", id) : "listbox"; | |||
|
|||
let [isControlled, setIsControlled] = React.useState(false); |
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.
Is there a reason we need new state for this? Could we not store this as a ref instead as we do lower in the tree? I don't believe we ever need it for rendering.
let isControlledRef = React.useRef(false);
// in ComboboxInput:
let isControlled = controlledValue != null;
React.useEffect(() => {
isControlledRef.current = isControlled;
}, [isControlled]);
// In ComboboxOption:
let { isControlledRef } = React.useContext(ComboboxContext);
let handleClick = () => {
onSelect && onSelect(value);
transition(SELECT_WITH_CLICK, { value, isControlled: isControlledRef.current });
};
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.
@chaance that's exactly what I did originally, but decided to go with state, since setting that ref would not trigger a re-render, so I figured state would be more safe.
It would almost certainly work either way though. If you think a ref would be better I could change to that.
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.
@chaance ok I've got it ref based now. I'm putting just the boolean in context, as opposed to the ref itself. If you'd like me to remove the updater function, and just store the ref object, lemme know and I will.
Thanks for the changes, looks good 👍 |
Here's the context. In the existing Storybook, go to combobox controlled (third one). Then repeatedly type in the box, and select something. Occasionally (roughly 1 in 5 times) you'll see the text of the thing you select flash in the input, before being cleared. To make this more pronounced, and easier to see, edit this code:
to this
This PR fixes that, by changing the value to null when the combobox is used in controlled mode.
Thank you for contributing to Reach UI! Please fill in this template before submitting your PR to help us process your request more quickly.
yarn test
).yarn start
).This pull request:
If creating a new package:
examples
directorysrc
directory with anindex.tsx
entry filestyle.css
file (if needed by the new package)