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

Urgent: Adjust component prop-types for use in MjmlAttributes #66

Conversation

IanEdington
Copy link
Contributor

@IanEdington IanEdington commented Oct 29, 2021

Currently the prop-types assume most components have children. This is
not the case in when used within the MjmlAttributes component.
This PR removes the isRequired from all mjml components so they can be
used inside the Atttributes.

This is an example email that is valid in MJML but not in mjml-react currently: https://mjml.io/try-it-live/k1kGrokirG2 (full email is at the bottom in case the link gets modified)

Disclaimer: I haven't used prop-types in ~4 years so I'm definitely not in the know of how to use them please correct me if I'm doing something wrong.

Why it's Urgent

This is urgent because I'm a maintainer on the DefinitelyTyped Types and the DefinitelyTyped maintainers wants us to conform to the types in this libraries prop-types. If we do it the way the prop-types are done currently we will throw a bunch of compile time errors for things that are allowed in MJML. ref: DefinitelyTyped/DefinitelyTyped#56254 (comment)

Example Email

<mjml>
  <mj-head>
    <mj-attributes>

      <mj-text>Head Components</mj-text>

      <mj-breakpoint width="50px" />
      <mj-font name="some-font" />
      <mj-attributes />
      <mj-html-attributes />
      <mj-preview />
      <mj-style inline="inline" />
      <mj-title />

      <mj-text>Body Components</mj-text>

      <mj-accordion mj-class="some-class" />
      <mj-button mj-class="some-class" />
      <mj-carousel mj-class="some-class" />
      <mj-column mj-class="some-class" />
      <mj-divider mj-class="some-class" />
      <mj-group mj-class="some-class" />
      <mj-hero mj-class="some-class" />
      <mj-image mj-class="some-class" />
      <mj-navbar mj-class="some-class" />
      <mj-raw mj-class="some-class" />
      <mj-section mj-class="some-class" />
      <mj-social mj-class="some-class" />
      <mj-spacer mj-class="some-class" />
      <mj-table mj-class="some-class" />
      <mj-text mj-class="some-class" />
      <mj-wrapper mj-class="some-class" />

      <mj-text>Weirder components that still work</mj-text>

      <mjml lang="en" />
      <mj-head />
      <mj-body mj-class="some-class" />

    </mj-attributes>
  </mj-head>
  <mj-body>
    <mj-section>
      <mj-wrapper>The red x to the left shows that validation is being done on this mjml</mj-wrapper>
      <mj-column>
        <mj-text mj-class="blue big">
          Hello World!
        </mj-text>
      </mj-column>
    </mj-section>
  </mj-body>
</mjml>

Currently the prop-types assume most components have children. This is
not the case in when used within the MjmlAttributes component.
This PR removes the `isRequired` from all mjml components so they can be
used inside the Atttributes.
@daliusd
Copy link
Contributor

daliusd commented Nov 4, 2021

Looks good to me.

@daliusd daliusd merged commit c1eb06a into wix-incubator:master Nov 4, 2021
@IanEdington IanEdington deleted the adjust-components-prototypes-for-use-in-attributes branch November 4, 2021 13:37
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.

None yet

2 participants