[Bug/Question] StyleManager - SectorsView - addToCollection() uses "name" attribute to build the ID and fails with several spaces or special characters

[arachnosoft]

Hi @artf ,

Using the pluginOpts property from the grapesjs.init() method, I customized the sectors displayed on the Style Manager (through the Newsletter plugin), like this:

grapesjs.init({
[...]
            plugins: ['gjs-plugin-ckeditor', 'gjs-blocks-basic', 'gjs-preset-newsletter', 'gjs-preset-webpage'],
            pluginsOpts: {
[...]
'gjs-preset-newsletter': {
                    styleManagerSectors: [
                        {
                            id: "dimensions",
                            name: "My Custom Label With Spaces"
                            buildProps: [
                              [...]
                            ],
[...]
}
]
}
}
);

The goal being to translate the labels into another language.

But I found that the "name" property is also used... to build the id attribute of each sector's HTML tag, with a custom prefix, like "gjs-".

The problem being when "name" contains several spaces: only the first one is being replaced, leading to an invalid HTML id attribute (with spaces), which can't be targeted through CSS to apply some custom styling on the Style Manager panel (see attached screenshot).

I opened grapes.js, and found this in the addToCollection() method:

    var view = new SectorView({
      model: model,
      id: this.pfx + model.get('name').replace(' ', '_').toLowerCase(),
      name: model.get('name'),
      properties: model.get('properties'),
      target: this.target,
      propTarget: this.propTarget,
      config: this.config
    });

As you may know, JavaScript's standard "replace" method is acting kinda weird by default, replacing only the first occurrence in the source string, if you don't explicitely set a RegExp with the g flag (unlike all other programming languages available on Earth, which take care of this automatically). Hence the bug.

Moreover, the "name" attribute can also contain special (accentuated) characters in some languages, and they could also break the ID as well, as you don't escape them.

If using the "name" attribute is intentional here, and not a copy/paste mistake (in case you meant model.get('id') instead of model.get('name')), what's the purpose of this?
Rather than using, say, an "id" property we could define ourselves? (and naturally avoiding to use any spaces or special characters with a such-named property).

You could use the "id" attribute if defined, and fallback to your current "name" attribute conversion if necessary (taking care to replace ALL spaces).

Therefore, I changed this line in grapes.js:
id: this.pfx + model.get('name').replace(' ', '_').toLowerCase(),
with:
id: this.pfx + model.get('id'),

And, in my code:
styleManagerSectors: [ { id: "dimensions", name: "My Custom Label With Spaces", [...] } ]

So my sectors' ID is now "gjs-sm-dimensions" as expected, and not "gjs-sm-my_custom label with spaces"

Subsidiary question: is it dangerous to keep this change while waiting for an official fix (or not) from you?

Thanks for your opinion.

-Maxime

[arachnosoft]

Here is the screenshot I mentioned earlier:

[artf]

If using the "name" attribute is intentional here, and not a copy/paste mistake (in case you meant model.get('id') instead of model.get('name')), what's the purpose of this?
Rather than using, say, an "id" property we could define ourselves? (and naturally avoiding to use any spaces or special characters with a such-named property).

Eh definitely not a wise choice, probably I was not yet using ids on that models. I'll change it in the next release.

Subsidiary question: is it dangerous to keep this change while waiting for an official fix (or not) from you?

I think, to make it more customize friendly, it would be a case to remove that id attribute and add a new class instead, so the final result should be class="gjs-sm-sector gjs-sm-sector__MYID"

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.