Faire / mjml-react

React component library to generate the HTML emails on the fly
https://www.npmjs.com/package/@faire/mjml-react
MIT License
375 stars 16 forks source link

Add `number` to `fontWeight` union #81

Closed nezort11 closed 1 year ago

nezort11 commented 1 year ago

fontWeight can be a number (e.g. 700, 400).

like

<div style={{ fontWeight: 700 }}>Some text</div>
emmclaughlin commented 1 year ago

Hi @nezort11 ! Thank you for contributing. This is a great find, however we should fix this for all components instead of just for MjmlText. We have a function in the script that tries to determine prop type https://github.com/Faire/mjml-react/blob/main/scripts/generate-mjml-react.ts#L120. The fontWeight is "number" but we don't check for that so it seems we fall back to the line 153 return.

To fix this we should add a line like:

if (mjmlAttributeType === "number") {
 return "string | number"
}

inside of the getPropTypeFromMjmlAttributeType function, and perhaps a test to confirm we can pass a number to the fontWeight prop without a typescript error.

IanEdington commented 1 year ago

It looks like mjml defines font-weight as a string: https://github.com/mjmlio/mjml/blob/85bc58e9e2a88cef7424dc2fce3d889f66fb4298/packages/mjml-text/src/index.js#L18

We typically want to stick with the mjml types but it seems that it's not enforced with their strict validation: https://mjml.io/try-it-live/dKaQw9yEj

Not positive what to do here yet.

IanEdington commented 1 year ago

oops I referenced the wrong issue in #85. It should have fixed #82