Social component icon colors remove displayed networks

[kickbk]

Reproduce like so: Go to your demo, add a social component. Add "google" and any other network, go and style the color of say Facebook's icon, and the networks we added disappear.

[kickbk]

Hey @artf, had a chance to check this out yet? ;)

[kickbk]

This is true for all components that have settings by the way. Changing the styles removes chosen settings.

[artf]

@kickbk apologize, but currently, I don't have time to work on this. PRs welcome

[kickbk]

@artf I dug some more into this. The reason things get messed up is because you merge attributes with styles in the mjml project. You added a listener for style change to init of coreMjmlModel, but you are not listening to changes of attributes. That was the first problem.

I broke down style/attributes change like so:

init() {
      const attrs = { ...this.get('attributes') };
      const style = { ...this.get('style') };

      for (let prop in style) {
        if (!(prop in attrs)) {
          attrs[prop] = style[prop];
        }
      }

      this.set('attributes', attrs);
      this.set('style', attrs);

      this.listenTo(this, 'change:style', this.handleStyleChange);
      this.listenTo(this, 'change:attributes', this.handleAttributeChange);
    },

    handleAttributeChange() {
      this.set('style', this.get('attributes'));
    },
    handleStyleChange() {
      this.set('attributes', this.get('style'));
    },
...

Next, I could find no reason to worry about grapes rendering default styles by changing style to 'style-default' since we override them as soon as we customize the settings. Also, by reverting to default style in the component we can now, for instance, see the default icon colors.

So now that our attributes and styles are correctly handled, I found no purpose for getAttrToHTML() If I'm not mistaken, all it is meant to do is remove default styles values from updated attributes. Since it's the only place this function is ran, I removed it and updated like so:

 toHTML(opts) {
      ...
      // Build the string of attributes
      let strAttr = '';
      let attr = this.get('attributes');
      ...
  };

I tested and so far it works well. However, I have noticed something that I haven't before. It seems that traits do not save empty values. So facebook-content updated as blank will remove the attributes (or "style", since they are copied over), and since the default value of the facebook-content trait is "Share", refreshing the page and loading the mjml from localstorage, the mjml will have no facebook-content attribute and this will result in the component rendering "Share" next to the Facebook button. Changing "value" to "default" in trait results in the same behavior.

I believe it would be best to allow nil/blank for attributes or add a "default" value of a trait that behaves differently than a set value.

So to sum up, right now the new issue I found SEEMS to be the only one I am facing when updating attributes of mjml components.

Could you please share your thoughts and perhaps better ideas or shed some light on something I may have missed?

[artf]

Closing as seems to be fixed