Closed caseyjhol closed 1 year ago
It's expected on the try it live as it's not the latest version but 4.13 should have a fix for that, so weird that filter doesn't do the trick... I'll try to see what's going on
On Wed, Mar 15, 2023 at 11:49 PM Casey Holzer @.***> wrote:
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:
- Go to https://mjml.io/try-it-live/components/social. Or create a file using:
- The link text has the following HTML:
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
— Reply to this email directly, view it on GitHub https://github.com/mjmlio/mjml/issues/2649, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAELHTOSZEUDIDEXSTERMF3W4JBRHANCNFSM6AAAAAAV4OYG2Q . You are receiving this because you are subscribed to this thread.Message ID: @.***>
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.
This should be covered by the isNil filter tho https://github.com/mjmlio/mjml/blob/master/packages/mjml-social/src/Social.js#L56-L81
This particular issue is related to SocialElement.js, where there's no check for that in getStyles
when retrieving text-padding.
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
Ah ha. This issue can be closed. This was fixed on August 2, 2022 (https://github.com/mjmlio/mjml/issues/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?
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 :(
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:
There is no padding on the
td
and the link has textcolor:#333333
,font-size:15px
, andline-height:22px
.Expected behavior The surrounding
td
should havepadding: 4px 4px 4px 0;
. The link text color should be#000
. The line-height should be'1'
.mj-social-element
has the following defaults:The relevant styles used for the link:
MJML environment (please complete the following information):