Banner Component Typescript Migration

[orrgottlieb]

TypeScript here we come!
We are migrating to typescript, we are doing so component by component

We would love some help in converting some of our component to typescript - we've created a README
https://github.com/mondaycom/monday-ui-react-core/blob/master/TYPESCRIPT_MIGRATION.md

in this PR we expect you to convert the following files

• Banner.jsx
• banner.jest.js
• BannerConstants.js

Good luck!

[suvnshr]

Hi @orrgottlieb, I would like to pick this up.

I would be following the patterns used in the .tsx and .ts files already present in the project.

[orrgottlieb]

Hi @suvnshr go for it!

[suvnshr]

Hey @orrgottlieb

I'm wanted you to review some changes before I commit them, coz I think I will have to make changes to Button.tsx and Icon.tsx:

The close button snippet used in Button.jsx, cannot be used as is in Button.tsx because of a lot of restrictions of types:

1. Button has a string type defined for children prop

• But I have seen a lot of Button snippets in the code where the children is not always string
• Adding a React.Element type to the Button's children prop fixes this.

2. iconSubComponentProps has only string type for the size prop , but CloseSmallProps has string | number for size props

• Adding a number to size prop in iconSubComponentProps fixes this

3. iconSubComponentProps has only React.MouseEvent<HTMLElement> for onClick mouse events , but CloseSmallProps extends React.SVGAttributes<SVGElement> which sends mouse event as React.MouseEvent<SVGElement>

• Adding a React.MouseEvent<SVGElement> to the mouse events of the onClick listener fixes this.

---

So my question is shall I go ahead and add the required types in Button.tsx and Icon.tsx?

I have ran all tests after making the above changes, and there were no failures.

PS: If I am not supposed to make the above changes then let me know how I can render that Button without triggering the above violations, thanks!

[orrgottlieb]

Hi @suvnshr thank you for the comments

1. please fix the way that you suggested adding the React.Element type, you may (if you want) replace the string type with it
2. awesome solution, go for it!
3. we are using HTMLElement quite a lot, can you please add a type there that it is either HTMLElement | SVGElement in the types folder here, thank you

As this migration process is a work in progress you can fix types according to new needs

[suvnshr]

Ok sure. Will do the changes.

Thanks!