-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Migrate switch button and spinner to TypeScript #4504
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
Migrate switch button and spinner to TypeScript #4504
Conversation
|
Beep boop! 🤖 Hey @kawamataryo, thanks for your PR! One of my human friends will review this PR and get back to you as soon as possible. Stay awesome! 😎 |
beerose
left a comment
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 see some type errors in declarations.d.ts file after extending HTMLAttributes with css prop. Could you look at this?
b57c053 to
5c15555
Compare
|
Deploy preview for hasura-docs ready! Built with commit 0b51c98 |
|
Review app for commit 5c15555 deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com |
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.
Type cast is not a solution here, because we are losing type information. props are not only SpinnerProps — they are SpinnerProps, StyledSpinnerProps and HTML div element props.
But, it's a fact that styled-components and TypeScript doesn't work well together, unfortunately.
There are types mismatches, for example, type of prop color from styled-system conflicts with div element's color.
Here is how to deal with it (in order to preserve type information):
- Crate component
Boxand interfaceBoxProps. You can move it to its own folder.
interface BoxProps extends Omit<React.ComponentPropsWithRef<'div'>, 'color'> {}
const Box = ('div' as any) as React.FC<BoxProps>;- Create an interface
StyledSpinnerOwnProps. It should be an interface, not a type, because again, usingtypeand&results in types conflicts.
interface StyledSpinnerOwnProps
extends ColorProps,
BorderProps,
TypographyProps,
LayoutProps,
SpaceProps,
ShadowProps {}- Declare
StyledSpinner:
export const StyledSpinner = styled(Box)<StyledSpinnerOwnProps>`
position: relative;
${color}
${border}
${typography}
${layout}
${space}
${shadow}
`;- Export
StyledSpinnerPropsas:
export interface StyledSpinnerProps extends BoxProps, StyledSpinnerOwnProps {}Check the whole code here.
- Then in
index.jsuse this type:
export interface SpinnerProps extends StyledSpinnerProps {
size: string;
}0b51c98 to
76e1e02
Compare
|
Review app for commit 76e1e02 deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com |
| import { StyledSwitchButton, StyledSlider } from './SwitchButton'; | ||
|
|
||
| export const SwitchButton = props => { | ||
| export const SwitchButton: React.FC = props => { |
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.
SwicthButton is accepting props:
| <StyledSwitchButton {...props}> |
Spinner component.
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 fix it.
| } from 'styled-system'; | ||
|
|
||
| interface BoxProps extends Omit<React.ComponentPropsWithRef<'div'>, 'color'> {} | ||
| const Box = ('div' as any) as React.FC<BoxProps>; |
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.
Can you move it to a separate file, for example in console/src/components/UIKit/atoms/Box/index.ts as it will be needed in other places as well?
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.
Right. I fix it.
76e1e02 to
67e6487
Compare
|
Review app for commit 67e6487 deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com |
beerose
left a comment
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.
Great! Approving.
|
Review app for commit 0c286ba deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com |
|
Review app for commit 45f42fe deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com |
|
Review app https://hge-ci-pull-4504.herokuapp.com is deleted |
|
Beep boop! 🤖 Awesome work @kawamataryo! All of us at Hasura ❤️ what you did. Thanks again 🤗 |
Description
Related issue #4314.
Migrated switch button and spinner to TypeScript.
Changelog
CHANGELOG.mdis updated with user-facing content relevant to this PR.Affected components
Related Issues
Related issue #4314.
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
GraphQL
Breaking changes