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

Components overwrite child attributes with null #2448

Closed
SebastianStehle opened this issue Mar 17, 2022 · 2 comments
Closed

Components overwrite child attributes with null #2448

SebastianStehle opened this issue Mar 17, 2022 · 2 comments
Labels

Comments

@SebastianStehle
Copy link
Contributor

SebastianStehle commented Mar 17, 2022

Describe the bug
It is a little bit harder to describe, but I found it when working on a port of mjml to C#:

It is general problem I think, but reproducible in the Social component.

The social element has the following property:

  static allowedAttributes = {
    'text-padding': 'unit(px,%){1,4}'
  }

  static defaultAttributes = {
    'text-padding': '4px 4px 4px 0'
  }

I am going to omit the other attributes because they are not relevant.

So when I render the mj-social-element standalone I have the following (simplified) output for the text:

<td style="vertical-align:middle;padding:4px 4px 4px 0;">
        <a> Facebook </a>
</td>

But the social-element also has the same attribute without a default value, which falls back to null.

So when the social component creates the attributes for the child, it uses this code:

getSocialElementAttributes() {
    const base = {}
    if (this.getAttribute('inner-padding')) {
      base.padding = this.getAttribute('inner-padding')
    }

    return [
      'text-padding',
      MORE PROPERTIES
    ].reduce((res, attr) => {
      res[attr] = this.getAttribute(attr)
      return res
    }, base)
  }

It basically create a object with a lot of null values and these null values seem to override the default values from the child component, which makes no sense to me. Somehow you might have thought about that in the inner-padding, but I think the general component behavior is just wrong here.

This means that when a social-element is rendered inside a social component it has no text-padding anymore.

To Reproduce

<mjml>
  <mj-body>
      <mj-social>
          <mj-social-element name="facebook" href="https://mjml.io/">
            Facebook
          </mj-social-element>
      </mj-social>
  </mj-body>
</mjml>

Expected behavior
Components should not override children null.

MJML environment (please complete the following information):

  • OS: -
  • MJML Version: 4.12.0
@iRyusa
Copy link
Member

iRyusa commented Mar 30, 2022

Clearly a bad side effect of inheritance that we wanted originally. We should filter null values instead 👍

@iRyusa
Copy link
Member

iRyusa commented Sep 4, 2022

Should be fixed in recent MJML version.

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

No branches or pull requests

2 participants