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

mj-social-element default attributes not applied #2649

Closed
caseyjhol opened this issue Mar 15, 2023 · 7 comments
Closed

mj-social-element default attributes not applied #2649

caseyjhol opened this issue Mar 15, 2023 · 7 comments

Comments

@caseyjhol
Copy link
Contributor

Describe the bug
The defaultAttributes for mj-social-element (and possibly other components) are not getting applied in the resulting HTML.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://mjml.io/try-it-live/components/social. Or create a file using:
<mjml>
  <mj-body>
    <mj-section>
      <mj-column>
        <mj-social font-size="15px" icon-size="30px" mode="horizontal">
          <mj-social-element name="facebook" href="https://mjml.io/">
            Facebook
          </mj-social-element>
          <mj-social-element name="google" href="https://mjml.io/">
            Google
          </mj-social-element>
          <mj-social-element name="twitter" href="https://mjml.io/">
            Twitter
          </mj-social-element>
        </mj-social>
      </mj-column>
    </mj-section>
  </mj-body>
</mjml>
  1. The link text has the following HTML:
<td style="vertical-align:middle;">
    <a href="https://www.facebook.com/sharer/sharer.php?u=https://mjml.io/" style="color:#333333;font-size:15px;font-family:Ubuntu, Helvetica, Arial, sans-serif;line-height:22px;text-decoration:none;" target="_blank"> Facebook </a>
</td>

There is no padding on the td and the link has text color:#333333, font-size:15px, and line-height:22px.

Expected behavior
The surrounding td should have padding: 4px 4px 4px 0;. The link text color should be #000. The line-height should be '1'.

mj-social-element has the following defaults:

  static defaultAttributes = {
    align: 'left',
    color: '#000',
    'border-radius': '3px',
    'font-family': 'Ubuntu, Helvetica, Arial, sans-serif',
    'font-size': '13px',
    'line-height': '1',
    padding: '4px',
    'text-padding': '4px 4px 4px 0',
    target: '_blank',
    'text-decoration': 'none',
    'vertical-align': 'middle',
  }

The relevant styles used for the link:

'tdText': {
    'vertical-align': 'middle',
    'padding'       : self.getAttribute('text-padding'),
},
'text'  : {
    'color'          : self.getAttribute('color'),
    'font-size'      : self.getAttribute('font-size'),
    'font-weight'    : self.getAttribute('font-weight'),
    'font-style'     : self.getAttribute('font-style'),
    'font-family'    : self.getAttribute('font-family'),
    'line-height'    : self.getAttribute('line-height'),
    'text-decoration': self.getAttribute('text-decoration'),
},

MJML environment (please complete the following information):

  • MacOS
  • v4.13.0
  • MJML CLI
@iRyusa
Copy link
Member

iRyusa commented Mar 16, 2023 via email

@caseyjhol
Copy link
Contributor Author

The flaw, I believe, has to do with how JS handles merged objects when keys have an associated value of undefined.

For example:

const defaultAttrs = {'text-padding': '4px'};
const attributesUndefined = {'text-padding': undefined};
const attributesEmpty = {};

let combined = {...defaultAttrs, ...attributesUndefined}; // {'text-padding': undefined}
combined = {...defaultAttrs, ...attributesEmpty}; // {'text-padding': '4px'}

It might make sense to filter out keys with undefined values before merging; otherwise they will always overwrite default attributes.

@iRyusa
Copy link
Member

iRyusa commented Mar 16, 2023

This should be covered by the isNil filter tho https://github.com/mjmlio/mjml/blob/master/packages/mjml-social/src/Social.js#L56-L81

@caseyjhol
Copy link
Contributor Author

This particular issue is related to SocialElement.js, where there's no check for that in getStyles when retrieving text-padding.

@iRyusa
Copy link
Member

iRyusa commented Mar 16, 2023

Yup but mj-social pass down few attribute to social element to override the "default" here https://github.com/mjmlio/mjml/blob/master/packages/mjml-social/src/Social.js#L100
Maybe... there's an issue down here https://github.com/mjmlio/mjml/blob/master/packages/mjml-core/src/createComponent.js#L68-L75

@caseyjhol
Copy link
Contributor Author

Ah ha. This issue can be closed. This was fixed on August 2, 2022 (#2448), but the fix was not included in v4.13.0: https://github.com/mjmlio/mjml/blob/v4.13.0/packages/mjml-social/src/Social.js.

Any ideas when we might see a new release?

@iRyusa
Copy link
Member

iRyusa commented Mar 16, 2023

Damn... I thought i've put it in 4.13.0 ... my bad
I hope to have some time to look at it by the end of next week :(

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

No branches or pull requests

2 participants