这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@kawamataryo
Copy link
Contributor

@kawamataryo kawamataryo commented Apr 21, 2020

Description

Related issue #4314.
Migrated switch button and spinner to TypeScript.

Changelog

  • CHANGELOG.md is updated with user-facing content relevant to this PR.

Affected components

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System
  • Tests
  • Other (list it)

Related Issues

Related issue #4314.

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No
  • Yes

Metadata

Does this PR add a new Metadata feature?

  • No
  • Yes

GraphQL

  • No new GraphQL schema is generated
  • New GraphQL schema is being generated:

Breaking changes

  • No Breaking changes
  • There are breaking changes:

@kawamataryo kawamataryo requested a review from a team as a code owner April 21, 2020 21:31
@hasura-bot
Copy link
Contributor

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! 😎

Copy link
Contributor

@beerose beerose left a 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?

@kawamataryo kawamataryo force-pushed the migrate-atoms-switch-button-and-spinner-to-ts branch from b57c053 to 5c15555 Compare April 23, 2020 20:43
@netlify
Copy link

netlify bot commented Apr 23, 2020

Deploy preview for hasura-docs ready!

Built with commit 0b51c98

https://deploy-preview-4504--hasura-docs.netlify.app

@hasura-bot
Copy link
Contributor

Review app for commit 5c15555 deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4504-5c15555c

@beerose beerose self-assigned this Apr 23, 2020
Copy link
Contributor

@beerose beerose left a 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):

  1. Crate component Box and interface BoxProps. 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>;
  1. Create an interface StyledSpinnerOwnProps. It should be an interface, not a type, because again, using type and & results in types conflicts.
interface StyledSpinnerOwnProps
  extends ColorProps,
    BorderProps,
    TypographyProps,
    LayoutProps,
    SpaceProps,
    ShadowProps {}
  1. Declare StyledSpinner:
export const StyledSpinner = styled(Box)<StyledSpinnerOwnProps>`
  position: relative;

  ${color}
  ${border}
  ${typography}
  ${layout}
  ${space}
  ${shadow}
`;
  1. Export StyledSpinnerProps as:
export interface StyledSpinnerProps extends BoxProps, StyledSpinnerOwnProps {}

Check the whole code here.

  1. Then in index.js use this type:
export interface SpinnerProps extends StyledSpinnerProps {
  size: string;
}

@kawamataryo kawamataryo force-pushed the migrate-atoms-switch-button-and-spinner-to-ts branch from 0b51c98 to 76e1e02 Compare April 26, 2020 19:52
@kawamataryo
Copy link
Contributor Author

@beerose

That was really helpful!! Thanks a lot.
I fixed it.

76e1e02

@hasura-bot
Copy link
Contributor

Review app for commit 76e1e02 deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4504-76e1e028

import { StyledSwitchButton, StyledSlider } from './SwitchButton';

export const SwitchButton = props => {
export const SwitchButton: React.FC = props => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SwicthButton is accepting props:

so we need to do the same thing as in the case of Spinner component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fix it.

67e6487

} from 'styled-system';

interface BoxProps extends Omit<React.ComponentPropsWithRef<'div'>, 'color'> {}
const Box = ('div' as any) as React.FC<BoxProps>;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I fix it.

6cba41e

@kawamataryo kawamataryo force-pushed the migrate-atoms-switch-button-and-spinner-to-ts branch from 76e1e02 to 67e6487 Compare May 7, 2020 21:37
@hasura-bot
Copy link
Contributor

Review app for commit 67e6487 deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4504-67e64879

@beerose beerose self-requested a review May 8, 2020 14:34
@beerose beerose mentioned this pull request May 14, 2020
46 tasks
Copy link
Contributor

@beerose beerose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Approving.

@beerose beerose requested a review from rikinsk May 15, 2020 12:09
@hasura-bot
Copy link
Contributor

Review app for commit 0c286ba deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4504-0c286bae

@hasura-bot
Copy link
Contributor

Review app for commit 45f42fe deployed to Heroku: https://hge-ci-pull-4504.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4504-45f42fee

@rikinsk rikinsk merged commit fffc65f into hasura:master May 19, 2020
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-4504.herokuapp.com is deleted

@hasura-bot
Copy link
Contributor

Beep boop! 🤖

GIF

Awesome work @kawamataryo! All of us at Hasura ❤️ what you did.

Thanks again 🤗

stevefan1999-personal pushed a commit to stevefan1999-personal/graphql-engine that referenced this pull request Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants