Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: setStyle & setComponents options parameter not taken into account #3176

Closed
mcottret opened this issue Dec 9, 2020 · 3 comments
Closed

Comments

@mcottret
Copy link

mcottret commented Dec 9, 2020

Version: 0.16.30

Are you able to reproduce the bug from the demo?

[x] Yes
[ ] No

Steps to reproduce:

  • Open the console
  • Execute editor.setComponents('', {avoidStore: true});
  • Execute editor.setStyle('', {avoidStore: true});
  • The "Stored ..." log still appears (see attached screenshot)

What is the expected behavior?

The options parameter should be taken into account. Passing the avoidStore option to true to setStyle & setComponents should prevent the update event from triggering.

What is the current behavior?

The options parameter is not taken into account. Passing the avoidStore option has no effect and an update event is still triggered.

Proposed solution:

I'm sorry I missed on this with the previous PR (for this issue), but there still seems to be 2 problems:

  • The opt object is not passed down to the clear & remove calls for setComponents, causing the avoidStore problem described above.
  • The CssComposer's handleChanges handler call signature does not match the one of the add & remove events: missing the 2nd collection parameter (which causes misreading of the opts parameter).

As always, I'd be happy to take care of the PR if this looks good to you !

Are you able to attach screenshots, screencasts or a live demo?

[x] Yes (attach)
[ ] No

Screenshot 2020-12-09 at 12 15 53 PM

@mcottret mcottret changed the title BUG: setStyle & setComponents options parameter not taken in account BUG: setStyle & setComponents options parameter not taken into account Dec 10, 2020
@artf
Copy link
Member

artf commented Dec 29, 2020

Unfortunately, due to the bad initial naming, avoidStore is intended to skip the UndoManager and not the Storage 😁
The good news, I had to introduce the new noCount option, in order to fix #3189, this will skip triggering the editor changes counter so, it won't trigger the Storage either.
That fix brought me also to update properly the signature in handleChanges from CssComposer, but I still need to fix a few other methods to make the whole thing work properly (including those you've suggested).

So, in the next release, the thing should work as you expect with the new noCount option, and, probably, I'll start using a new noUndo option (leaving avoidStore to avoid any breaking change).

ps. as always, thanks for your help ❤️

@artf artf added this to To do in Release v0.16.34 via automation Dec 29, 2020
@artf artf closed this as completed in 57a5138 Dec 30, 2020
Release v0.16.34 automation moved this from To do to Done Dec 30, 2020
@mcottret
Copy link
Author

mcottret commented Jan 7, 2021

Hello @artf !

I'm sorry to reopen this issue but the setStyle issue still seems to be present.

Redoing the steps above, the "Stored ..." log still appears while calling setStyle with the noCount option.

I'm seeing the fixed handleChanges handler still uses the avoidStore option, but the avoidStore option does not seem to work either.

I didn't look deeper into this so I couldn't identify why the issue persists, but let me know if you want me to :)

Everything works fine with setComponents on the other hand, thanks a lot for that :) !

@artf artf reopened this Jan 25, 2021
@artf artf added this to To do in Release v0.16.41 via automation Jan 25, 2021
@artf
Copy link
Member

artf commented Jan 25, 2021

Thanks @mcottret the fix is ready for the next release

@artf artf closed this as completed in bc0983d Jan 28, 2021
Release v0.16.41 automation moved this from To do to Done Jan 28, 2021
vijayshukla30 pushed a commit to vijayshukla30/grapesjs that referenced this issue Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants