-
Notifications
You must be signed in to change notification settings - Fork 184
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
[NO JIRA][BpkLoadingButton]: Updating TS types and removing propTypes ahead of React 18 #3588
Conversation
… ahead of React 18
Visit https://backpack.github.io/storybook-prs/3588 to see this build running in a browser. |
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.
Nice! Goodbye proptypes!!
@@ -156,39 +156,4 @@ const BpkLoadingButton = (props: LoadingProps) => { | |||
); | |||
}; | |||
|
|||
BpkLoadingButton.propTypes = { | |||
children: PropTypes.node.isRequired, | |||
className: PropTypes.string, |
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.
Should classname
be included in the destructed props above?
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.
Thats a good catch! Having a deeper look, its not actually used in the component itself! So I am wondering if this was added for typing in the past but would just follow ...rest
and go down to the BpkButton.
AFAIK flow/propTypes didn't have the concept of merging props with that of other components 🤔
@@ -16,9 +16,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
/* @flow strict */ |
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.
Hey Ollie could you please explain the removals of flow strict
and the changings of toMatchSnapshot
?
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.
As this file uses Typescript having flow in here does nothing and flow isn't installed in the repo anymore for a long time.
In terms of the snapshots, moved away from these as they aren't reliable and to use a more reliable testing over snapshots, something we advocate for in Skyscanner
icon?: ReactElement<any>, | ||
iconPosition: string, | ||
iconPosition?: string, |
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.
iconPosition?: string, | |
iconPosition?: ICON_POSITION.TRAILING || ICON_POSITION. LEADING, |
Visit https://backpack.github.io/storybook-prs/3588 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3588 to see this build running in a browser. |
As part of the migration to React 18 when running tests we found errors that were being through due to the mix of TS types and and propTypes which are deprecated and discouraged since React 17
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md