[Menu] Only require onCheckedChange to handle boolean values #1778
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes using CheckboxItem with TypeScript a bit simpler, as it
doesn't require state managed up the tree to allow indeterminate states
if it doesn't want to. More practically, it makes this allowed:
const [checked, setChecked] = useState(false);
<CheckboxItem
checked={checked}
onCheckedChange={setChecked}
/>
Previously that would result in a type error, because `checked` didn't
permit indeterminate states. However, CheckboxItem never actually sets
itself into an indeterminate state -- that can only be set via props --
meaning we shouldn't have to support that case if we don't want to.
|
Hey @cpmsmith, thanks for your contribution.
You are absolutely right! In fact this means the types are currently sort of lying, because
Yep, that makes sense. |
benoitgrelard
approved these changes
Feb 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This makes using
CheckboxItemwith TypeScript a bit simpler, as it doesn't require state managed up the tree to allow indeterminate states if it doesn't want to. More practically, it makes this allowed:Previously that would result in a type error, because
setCheckeddidn't permit indeterminate states. However,CheckboxItemnever actually sets itself into an indeterminate state—that can only be set via props—meaning we shouldn't have to support that case if we don't want to.I've marked this as a patch version because, despite changing a type definition, function types are contravariant in TypeScript, i.e. any function which is assignable to
(value: boolean | "indeterminate") => voidis also assignable to(value: boolean) => void, so I don't think this should break any existing usage.You can see the current state of things in this CodeSandbox, as well as the workaround required if you only want your state to be boolean.