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

Asset manager delete problems #549

Closed
duskhacker opened this issue Nov 20, 2017 · 7 comments
Closed

Asset manager delete problems #549

duskhacker opened this issue Nov 20, 2017 · 7 comments
Labels

Comments

@duskhacker
Copy link

In the Asset Manager, when the x to remove an asset is clicked, it appears to fire both the onDelete event AND the onClick event. This puts the asset you just deleted into the target. Furthermore, one cannot click another asset to replace it in the target until you close the asset manager window and re-open it.

If the user is not very careful, they can get the document into an inconsistent state that will not load properly from the server.

Any pointers on how to fix this are welcome.

@ryandeba
Copy link
Contributor

Hi @duskhacker,

I was able to recreate the issue of the onClick event firing when clicking on the X (jsfiddle in case anyone else cares to see it: https://jsfiddle.net/8v946q4b/3/). Sorry I don't have a lot of time to debug this myself at the moment (I may have time to research this tomorrow if you're still having trouble), but it appears to me to be a bug that should get fixed in the AssetImageView object. I see the onRemove method starts by calling e.stopPropagation(), which I believe is attempting to prevent the onClick method from firing, but that doesn't appear to be working if that is indeed the intent. My guess is that onClick is firing before onRemove for some reason, but I'm not sure this is the case nor why that would be happening.

@duskhacker
Copy link
Author

I may have time to research this tomorrow if you're still having trouble

@ryandeba Thanks for the followup! This is one of my outstanding problems with GrapesJS. I know I'm not going to be able to fix it in a reasonable time-frame, I'm not familiar with the code-base enough to debug effectively so I'll have to rely on your expertise. Currently, it is not a show-stopper I have lots of stuff to do before this goes into production. But if it can be fixed in the coming days that would be great!

@ryandeba
Copy link
Contributor

ryandeba commented Nov 22, 2017

@duskhacker - I've submitted a pull request that should fix this bug

#560

@artf artf closed this as completed in e84c34c Nov 23, 2017
@artf
Copy link
Member

artf commented Nov 23, 2017

Merged, HUUUGE thanks Ryan 🥇

@duskhacker
Copy link
Author

@ryandeba: Thank you!! 💯

@ryandeba
Copy link
Contributor

No problem! I'm happy to help out

@lock
Copy link

lock bot commented Sep 18, 2019

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.

@lock lock bot added the outdated label Sep 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants