Issue #1753Opened January 24, 2019by arachnosoft0 reactions

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

Question

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-<namePropertyInLowercase>".

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

Answers (3)

arachnosoftJanuary 24, 20190 reactions

Here is the screenshot I mentioned earlier:

grapesjs id attribute bug report

artfJanuary 27, 20190 reactions

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[bot]January 27, 20200 reactions

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.

Related Questions and Answers

Continue research with similar issue discussions.

Paid Plugins That Match This Issue

Curated by issue keywords and label relevance to help you ship faster.

View all plugins

Loading paid plugin recommendations...

Browse Plugin Categories

Jump directly to plugin category pages on the marketplace.