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(Banner): Migrated from JS to TS #917

Merged
merged 3 commits into from
Oct 7, 2022

Conversation

suvnshr
Copy link
Contributor

@suvnshr suvnshr commented Oct 4, 2022

  • Migrated Banner to Typescript
  • Migrated BannerConstants to Typescript
  • Migrated banner.jest to Typescript
  • Replaced type string with type ReactNode of children prop in Button
  • Added type number to size prop of iconSubComponentProps
  • Created a new type for onClick prop of iconSubComponentProps

Related issue: #894

Basic

  • Used plop (npm run plop) to create a new component.
  • PR has description.
  • New component is functional and uses Hooks.
  • Component defines PropTypes.

Style

  • Styles are added to NewComponent.modules.scss file inside of the NewComponent folder.
  • Component uses CSS Modules.
  • Design is compatible with Monday Design System.

Storybook

  • Stories were added to /src/NewComponent/__stories__/NewComponent.stories.js file.
  • Stories include all flows of using the component.

Tests

+ Migrated `Banner` to Typescript
+ Migrated `BannerConstants` to Typescipt
+ Migrated `banner.jest` to Typescipt
+ Replaced type `string` with type `ReactNode` of `children` prop in `Button`
+ Added type `number` to `size` prop of `iconSubComponentProps`
+ Created a new type for `onClick` prop of `iconSubComponentProps`
@suvnshr
Copy link
Contributor Author

suvnshr commented Oct 4, 2022

@orrgottlieb Here you go ❇️

Let me know if any changes are required.

Copy link
Contributor

@orrgottlieb orrgottlieb left a comment

Choose a reason for hiding this comment

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

@suvnshr Awesome job!
I'm approving as the comments are small, please tag me once you do these changes
again, really awesome job!

src/components/Banner/Banner.tsx Outdated Show resolved Hide resolved
src/components/Banner/Banner.tsx Outdated Show resolved Hide resolved
src/components/Button/Button.tsx Show resolved Hide resolved
src/components/Icon/Icon.tsx Show resolved Hide resolved
@suvnshr
Copy link
Contributor Author

suvnshr commented Oct 6, 2022

@orrgottlieb I've made the required changes.

Copy link
Contributor

@orrgottlieb orrgottlieb 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 an awesome job! and thank you @suvnshr for the contribution, we will merge and add the appropriate label

P.S
If you have any feedback on the package.- i would love it if you can write it to me
thank you

@orrgottlieb orrgottlieb linked an issue Oct 7, 2022 that may be closed by this pull request
3 tasks
@orrgottlieb orrgottlieb merged commit ef72f0e into mondaycom:master Oct 7, 2022
@suvnshr
Copy link
Contributor Author

suvnshr commented Oct 12, 2022

Thanks for the kind words @orrgottlieb

overall I liked the design system.

Wanted to ask you whether the Banner component is still in development?

coz I didn't see its doc in the storybook

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Banner Component Typescript Migration
2 participants