GrapesJS / grapesjs

Free and Open source Web Builder Framework. Next generation tool for building templates without coding
https://grapesjs.com
BSD 3-Clause "New" or "Revised" License
22.38k stars 4.06k forks source link

[QUESTION] ID changes when importing html or passing html in components prop in config. #1875

Closed simplecommerce closed 5 years ago

simplecommerce commented 5 years ago

Hi,

I built a component and block that I can drag and drop in the canvas.

My issue is that some styles rely on the ID. I know this might not be ideal, but it's like this at the moment.

My problem is when I reload the editor with my previous values by passing it in the components prop, all my ID's are changed.

Is there a way to prevent this?

Like, only change ID's for newly added blocks and not imported code or existing code?

I tried searching but could not see an option.

Thanks!

simplecommerce commented 5 years ago

Anyone have any idea for this issue?

My issue happens when I use for example, bootstrap tabs, in order to make them work and load the right tab, it uses the ID, but since the ID is being regenerated when I load the editor with my components again (for editing), the tabs no longer work, since all ID are replaced and incremented.

Initial drag and drop.

<div tabs-container="" id="iqwah">
  <ul role="tablist" class="nav nav-tabs">
    <li class="nav-item">
      <a id="home-tab-3" data-toggle="tab" href="#home" role="tab" aria-controls="home" aria-selected="true" class="nav-link active">Tab 1</a>
    </li>
    <li class="nav-item">
      <a id="profile-tab-3" data-toggle="tab" href="#profile" role="tab" aria-controls="profile" aria-selected="false" class="nav-link">Tab 2</a>
    </li>
    <li class="nav-item">
      <a id="contact-tab-3" data-toggle="tab" href="#contact" role="tab" aria-controls="contact" aria-selected="false" class="nav-link">Tab 3</a>
    </li>
  </ul>
  <div class="tab-content">
    <div role="tabpanel" class="tab-pane fade show active" id="home-3" aria-labelledby="home-tab">
    </div>
    <div role="tabpanel" class="tab-pane fade" id="profile-3" aria-labelledby="profile-tab">
    </div>
    <div role="tabpanel" class="tab-pane fade" id="contact-3" aria-labelledby="contact-tab">
    </div>
  </div>
</div>

Reload

<div tabs-container="" id="iqwah">
  <ul role="tablist" class="nav nav-tabs">
    <li class="nav-item">
      <a id="home-tab-3" data-toggle="tab" href="#home" role="tab" aria-controls="home" aria-selected="true" class="nav-link active">Tab 1</a>
    </li>
    <li class="nav-item">
      <a id="profile-tab-3" data-toggle="tab" href="#profile" role="tab" aria-controls="profile" aria-selected="false" class="nav-link">Tab 2</a>
    </li>
    <li class="nav-item">
      <a id="contact-tab-3" data-toggle="tab" href="#contact" role="tab" aria-controls="contact" aria-selected="false" class="nav-link">Tab 3</a>
    </li>
  </ul>
  <div class="tab-content">
    <div role="tabpanel" class="tab-pane fade show active" id="home-3" aria-labelledby="home-tab">
    </div>
    <div role="tabpanel" class="tab-pane fade" id="profile-3" aria-labelledby="profile-tab">
    </div>
    <div role="tabpanel" class="tab-pane fade" id="contact-3" aria-labelledby="contact-tab">
    </div>
  </div>
</div>

As you can see, my id's have changed.

sbiwaichoon commented 5 years ago

same issue here.if i have use that id in code, id change when(id-2) reload template to grapejs (auto added "-2" to my id) , if i dont use that id in any part of my code, is fine, id wont change

simplecommerce commented 5 years ago

Ok so since there didn't seem to be any options to prevent this, I had to overwrite the built-in functions in this method in order to prevent the ID being overwritten.

I placed this code after initializing the editor.

    editor.DomComponents.Component.createId = function(model) { 
      const list = comps.Component.getList(model);
      let { id } = model.get('attributes');
      let nextId;

      if (id) {               
        // only commented this line, to keep the original id.
        nextId = id//comps.Component.getIncrementId(id, list);

        model.setId(nextId);
      } else {
        nextId = comps.Component.getNewId(list);
      }

      list[nextId] = model;
      return nextId;
    }    

    // basically just removed the whole code.
    editor.DomComponents.Component.checkId = function() {}

There was another issue I noticed while investigating this, it seems then doing editor.DomComponents.clear(), it only clears the components, but not the list of ID's that were generated.

Had to add this to fix it.

    editor.DomComponents.clear();
    editor.DomComponents.componentsById = {};  // this here

Don't know if it was intentional or not.

artf commented 5 years ago

Hi Kheang and thanks for this report. Unique IDs on components are important for the editor in the same way as IDs on elements in HTML documents.

The Component.checkId method was created mainly to prevent duplicated IDs created from Blocks, check the example below:

editor.BlockManager.add('simple-block', {
    label: 'My block',
    content: `
        <div id="my-component">
            My content
        </div>
        <style>
            #my-component {
                padding: 10px;
                color: red;
            }
        </style>
    `
});

Here the situation is quite easy to solve, but obviosly is not the same here:

editor.BlockManager.add('simple-block', {
    label: 'My block',
    content: `
        <div id="my-component">
            <span>My content</span>
            <div class="cmp1" data-some-attribute="#linked-id">Component 1</div>
            <div class="cmp2" id="linked-id">Component 2</div>
        </div>
        <style>
            #my-component {
                padding: 10px;
                color: red;
            }
        </style>
    `
});

Here I have to keep the connection between Component 1 and Component 2. In cases like this I had a well defined components for them so it was solved with some listener on change:attribute:id but obviously it'd be cool to solve this without the need to create custom components.

Without editing editor.DomComponents.Component methods I'd suggest doing this:

editor.on('block:drag:stop', component => {
      if (!component) return;
      const cmp1 = component.find('.cmp1')[0];
      const cmp2 = component.find('.cmp2')[0];
      cmp1 && cmp2 && cmp1.addAttributes({
        'data-some-attribute': `#${cmp2.getId()}`
      })
});

It's not perfect as it involves a global event listener (so you might need also to check if it's the correct component to edit) but at least it should work.

Probably another alternative to global events would be to add some kind of hooks on block definition:

editor.BlockManager.add('simple-block', {
    ...
    onSelect(block) {...},
    onDrop(component, block) {...},
    etc.
})

What do you think about it? Might it help?

There was another issue I noticed while investigating this, it seems then doing editor.DomComponents.clear(), it only clears the components, but not the list of ID's that were generated.

Yep, you're right, I'll fix it for the next release

simplecommerce commented 5 years ago

@artf Hi, thanks for your reply, I appreciate you taking the time to answer this.

I totally understand about the ID generated being unique, that is not an issue to me and makes sense.

Your method would work for the tab issue and any components that we would add which relies on ID.

The thing that makes it strange, is that we have an ID trait, that we can customize ourselves without touching the code directly, but upon reload into the canvas, all of them are changed.

Would it not make sense, that if an ID is changed by an end-user, that it should remain this way, instead of regenerating it on every load or import into the canvas?

Also, the main reason why this was also causing us an issue, is because our integrators define their CSS using SCSS with an ID as the parent element and then adds their styles and/or classes inside the parent bracket. They do it this way because it allows them to control some aspect of the styles and make sure there are no confusion, since the ID should be unique.

Here is an example:

#capsules_articles_2019 {
    background-color: #fff;

    .articles_ext {
        display: none;
    }

    .flex-parent-1 {
        display: flex;
        align-items: stretch;
        flex-direction: row;
        flex-wrap: wrap;
        padding: 0 10px;
    }
}

But because of this ID being regenerated, it would prevent them from doing so and force them on using only classes to define their CSS.

That is why in the meantime, I had to overwrite the method that generates a new ID every time, as to preserve existing stylesheets without having to redo them completely. (time issue)

But I personally still think that if the user sets an ID or imports code that has its own ID's defined, it shouldn't change, generated ID's should only be for components that you drag and drop which weren't already part of existing code, and should have their ID's validated against the canvas's code, maybe?

But I am not sure as I do not know the whole implication of this.

Thanks again!

artf commented 5 years ago

Would it not make sense, that if an ID is changed by an end-user, that it should remain this way, instead of regenerating it on every load or import into the canvas?

Wait, if you change the ID from the trait, store it and then reload it, AND if the ID is unique then it will be kept (it will be refreshed only if it's not unique). It works like this also on the main demo

simplecommerce commented 5 years ago

@artf Is it possible that it only works when you store and load using the Storage but now when you pass the saved components via the components prop in the editor when initializing?

Because in my use case, I am not using the Storage manager, I am handling the saving and loading myself, and load using the components prop when initializing the editor.

Also, the import code is often used to do adjustments to the code, so via this method the ID changes also.

Just pointing some use cases.

artf commented 5 years ago

@artf Is it possible that it only works when you store and load using the Storage but now when you pass the saved components via the components prop in the editor when initializing? Because in my use case, I am not using the Storage manager, I am handling the saving and loading myself, and load using the components prop when initializing the editor.

No, it should work in the same way I've described before. Are you able to create a reproducible demo?

simplecommerce commented 5 years ago

@artf Ok I will get one up and get back to you, thanks!

simplecommerce commented 5 years ago

@artf You were right, the issue is not linked to the checkId function or the other, it only seems to be this part:

editor.DomComponents.componentsById = {}, if I do not do this, it will keep regenerating the ID's, which normally will be fixed in the next release.

But I figured out another problem I am having, which is related to this.

I created components and blocks using the API after the editor is initialized, as per the docs.

I pass autorender: 0 to the editor and later on call:

editor.render();

But my problem is that my components are not being rendered, they are only rendered if I add the following line before the editor.render().

editor.getModel().loadOnStart();

But the issue, causes the problem of ID's being regenerated.

Here is a sample code to produce the error. When loading you will see the test block has no styles or anything, but if you click on the update canvas button, which is the same block, the component will render correctly.

https://codesandbox.io/s/zz2nz90r7x

Appreciate the input!

artf commented 5 years ago

I created components and blocks using the API after the editor is initialized, as per the docs. I pass autorender: 0 to the editor and later on call: editor.render(); But my problem is that my components are not being rendered, they are only rendered if I add the following line before the editor.render().

Ok I heard already about it but unfortunately wasn't able to find time to debug it, so for now you have to put your custom component declaration via plugins, it's the only way to make them work properly

simplecommerce commented 5 years ago

@artf Thanks for the feedback, for now I am clearing all my components and canvas before doing the loadOnStart call and it seems to be ok, I will wait for a fix later on, thanks again!

artf commented 5 years ago

@simplecommerce new release published

lock[bot] commented 4 years ago

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.