-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Value typing for Checkbox ToggleButtonGroup #3584
Conversation
Checkbox ToggleButtonGroup holds array of T rather than singleton.
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.
see suggestion
Co-Authored-By: mhermher <mhermher@live.com>
value?: T; | ||
defaultValue?: T; | ||
value?: T[]; | ||
defaultValue?: T[]; |
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.
wait no, these can be T
or T[]
, depending on whether the type is checkbox
or radio
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.
Checkbox and Radio Props have separate typings. The generalized typing is a union of those two. Is the radio supposed to be single-value and the checkbox multi-value? In that case, only the checkbox would have T[]. That is from my understanding at least.
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.
yup those all come from ToggleButtonRadioProps
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.
ha sorry, didn't notice that in the github diff view. tbf tho, the component always allows T or T[] regardless of type, but i'd agree it's better to have the types be stricter
value?: T; | ||
defaultValue?: T; | ||
value?: T[]; | ||
defaultValue?: T[]; |
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.
yup those all come from ToggleButtonRadioProps
Sorry, it says I dismissed your review Taion. Idk what that means, but a test was failing so I pushed another commit to fix that. That might have been the cause of that. |
Checkbox ToggleButtonGroup holds array of T rather than singleton.