-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(SegmentedControl): allow for passing stricter type for options #7051
base: develop
Are you sure you want to change the base?
feat(SegmentedControl): allow for passing stricter type for options #7051
Conversation
Generate changelog in
|
@@ -83,133 +83,151 @@ export interface SegmentedControlProps | |||
small?: boolean; | |||
} | |||
|
|||
// This allows the ability to pass a more strict type for `options`/`onValueChange` | |||
// i.e. <SegmentedControl<Intent> /> | |||
interface ReactFCWithGeneric extends React.FC<SegmentedControlProps> { |
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.
Nit this is a specific interface for SegmentedControl
so lets name it something in accordance with that.
interface ReactFCWithGeneric extends React.FC<SegmentedControlProps> { | |
interface GenericSegmentedControl extends React.FC<SegmentedControlProps> { |
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 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.
Also just as an FYI, I created this type to match the fact that all other components in this repo specify the type React.FC
. But really you don't need to manually add a type at all, you could just have
export const SegmentedControl = React.forwardRef(
<T extends string>(props: SegmentedControlProps<T>, ref: React.ForwardedRef<HTMLDivElement>) => {
So let me know if you like that better?
}; | ||
export const SizeSelect: React.FC<SizeSelectProps> = ({ label, size, optionLabels, onChange }) => ( | ||
<FormGroup label={label}> | ||
<SegmentedControl<Size> |
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.
Out of curiosity, in these types of cases if you omit <Size>
is the generic type parameter properly inferred from the options
array? I'm guessing not because there's no as const
.
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.
I'm guessing not because there's no as const.
Correct, it can't be inferred from the options
in that case. But it is inferred from the type of the value
prop as well as the arg of the onChange
prop
{ label: optionLabels[1], value: "regular" }, | ||
{ label: optionLabels[2], value: "large" }, | ||
]} | ||
onValueChange={onChange} |
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.
Another point of curiosity (since we ran into this issue with another generic component recently), if you provide an arrow function here, does the type of the argument get properly inferred?
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.
Yes. If you specify a type in the generic, it will make sure onChange
arg has the proper type as well as the options
. If you don't specify a generic, the type of the onChange
arg may determine what the options
have to be.
Checklist
Changes proposed in this pull request:
Allow for stricter type on
SegmentedControl
than juststring
, which reduces the need for type casting in many cases (such asonChange
call).For example, you can still have just
as before, for general string Options, or you can have
to make sure that all passed option values, and the
onChange
, have theOptionsEnum
type.Reviewers should focus on:
Improved type casting abilities, and mainly, the reduction of having to type cast.
Screenshot
No visual changes.