-
Notifications
You must be signed in to change notification settings - Fork 514
chore: update types for react #6191
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,27 +8,14 @@ import React from 'react'; | |
|
|
||
| import ClayButton from './Button'; | ||
|
|
||
| type ButtonAria = | ||
| | { | ||
| /** | ||
| * Define a value that labels the button. | ||
| */ | ||
| 'aria-label': string; | ||
| 'aria-labelledby'?: never; | ||
| } | ||
| | { | ||
| /** | ||
| * Define a value that labels the button. | ||
| */ | ||
| 'aria-label'?: never; | ||
| 'aria-labelledby': string; | ||
| }; | ||
|
|
||
| interface ICommonProps | ||
| extends Omit< | ||
| React.ComponentProps<typeof ClayButton>, | ||
| 'aria-label' | 'aria-labelledby' | ||
| > { | ||
| 'aria-label'?: string; | ||
|
|
||
| 'aria-labelledby'?: string; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old type looked like it was requiring either aria-labelledby or aria-label to be included while also not allowing both to be added at the same time. This doesn't look like this will have the same behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason with the updated type libraries, it no longer allowed them to be mutually exclusive. I'm not exactly why or what type changed in the react definitions, but I figured it might just be simpler to make them both optional types instead. |
||
| /** | ||
| * Path to the location of the spritemap resource. | ||
| */ | ||
|
|
@@ -40,7 +27,7 @@ interface ICommonProps | |
| symbol: string; | ||
| } | ||
|
|
||
| export type Props = ICommonProps & ButtonAria; | ||
| export type Props = ICommonProps; | ||
|
|
||
| const ClayButtonWithIcon = React.forwardRef<HTMLButtonElement, Props>( | ||
| ({monospaced = true, spritemap, symbol, ...otherProps}: Props, ref) => ( | ||
|
|
||
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.
What required this change?
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.
Because
React.Children.mapno longer accepts the function type, so we have to guard against that.Additionally we have to add Function as a suspected type to hasItems because in ClayAutocomplete the children prop can be a function
Essentially there were gaps in the previous types, so I think it just got more strict.