Skip to content
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

refactor(typescript): convert Grid, FlexGrid and children to TypeScript #13074

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

lewandom
Copy link
Contributor

@lewandom lewandom commented Feb 2, 2023

Closes #12537.

Convert React Grid, FlexGrid and related components (Column, ColumnHang, Row) to .tsx to add TypeScript support.

Changelog

New

  • added TypeScript types for all the components and their props in the scope

Changed

  • converted component files from *.js to *.ts(x)

Testing / Reviewing

The component imports for the storybook should be valid and rendering. In a TypeScript environment, you should be able to import and reference all public components (Grid, FlexGrid, Column, ColumnHang, Row) and their props typings. The use of as attribute should allow for specifying both intrinsic and custom components as a replacement to the default base component (div). When used in TypeScript setup, type validation / hints for attributes coming from custom base component should work.

@lewandom lewandom requested a review from a team as a code owner February 2, 2023 15:03
@netlify
Copy link

netlify bot commented Feb 2, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a9ab077
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/63ede16f09f86600079925e4
😎 Deploy Preview https://deploy-preview-13074--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Feb 2, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit a9ab077
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/63ede16f3304ea000869f8df
😎 Deploy Preview https://deploy-preview-13074--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @lewandom!
In this case we're not looking to remove CSSGridColumn's ability to accept the span prop, we'd want to allow the Column component to receive everything the CSSGridColumn might need so that it's able to pass it on instead.
And we do want to bring back in the spanClassName class and getClassNameForSpan function into this PR 🙏🏻
Other than that LGTM!

@lewandom
Copy link
Contributor Author

lewandom commented Feb 2, 2023

@francinelucca thanks for reviewing! Just wanted to double-confirm: the span attribute is not part of the original PropTypes. Should it be added there? Right now because it's not in PropTypes, it doesn't show in storybook. Should it be added there? Also, is that / should it be marked as deprecated, or is a regular supported attribute?

@francinelucca
Copy link
Collaborator

@lewandom not deprecated. I think if TS can let us get away with not specifically adding it to the proptypes but still allow passing it through ...rest we can continue to do that since it's a subcomponent's prop and we have precedence for that on the library. If that's not possible then explicitly adding it should be fine too.

Unless @tay1orjones you think we should explicitly add it anyway?

@lewandom
Copy link
Contributor Author

lewandom commented Feb 3, 2023

@francinelucca the span attribute restored. It needs to be part of ColumnProps type, but not added to PropTypes. So far it's not visible in storybook.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

This looks good to me. Just one comment/question about if we could consolidate the RenderAsProps with the existing helper in common.d.ts

Comment on lines 37 to 41
export type RenderAsProps<BaseProps, T> =
| RenderAsDefaultProps<BaseProps>
| (T extends keyof JSX.IntrinsicElements
? RenderAsIntrinsicProps<BaseProps, T>
: RenderAsCustomComponentProps<BaseProps, T>);
Copy link
Member

Choose a reason for hiding this comment

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

This is a dense file that would benefit from extensive commenting to explain the structure and purpose of each type.

Also, a helper already exists for as props:

/**
* For "as" props. Creates an "as" property that supports native html tags such as 'span', 'a', 'button' as well as custom functional components
* All native props for the supplied html tag/component are inferred as part of the base component props, allowing us to use props like `href` on an 'a' element ect
*/
export type PolymorphicProps<Element extends React.ElementType, Props> = Props &
Omit<React.ComponentProps<Element>, 'as'> & {
as?: Element;
};

Should that be extended with this? Consider lifting this up out of the Grid component file and into that common file. The as prop pattern is used across 21 different components. Searching/find all as: PropTypes in the codebase you can see the different ways it's used. Having a cohesive approach for them would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tay1orjones thanks for pointing me at the PolymorphicProps type, I wasn't aware of it's existence. I started with the DefinitelyTyped types and wasn't aware we have such a type already available. In fact, I was also not satisfied what DefinitelyTyped provided and was searching for a method to use ComponentProps type, but couldn't find one. PolymorphicProps are clean and elegant.

I've removed RenderAsProps entirely as it was not necessary anymore and rewritten all components to use PolymorphicProps. Please review once more.

@kodiakhq kodiakhq bot merged commit e4e1c0f into carbon-design-system:main Feb 16, 2023
remolueoend pushed a commit to remolueoend/carbon that referenced this pull request Feb 17, 2023
…pt (carbon-design-system#13074)

* refactor(typescript): convert Grid, FlexGrid and children to TypeScript

* fix(Grid): restore span attribute support

* fix(typescript): use PolymorphicProps
@lewandom lewandom deleted the typescript-grid branch November 6, 2023 11:25
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.

Add TypeScript types to Grid, FlexGrid, Column, ColumnHang, Row
3 participants