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

Fix custom plugin default command overriding #356

Merged

Conversation

arthuralmeidap
Copy link
Contributor

@artf ,
Here is my fix. Take a look and tell me what you think.

@arthuralmeidap
Copy link
Contributor Author

I forgot to mention but this PR is related for the issue #355

@artf
Copy link
Member

artf commented Oct 3, 2017

Hi Arthur and thanks for the contribution, by the way, I can't accept it as it's important that onLoad triggers after plugins and the reason is just above in the comment

// Execute `onLoad` on modules once all plugins are initialized.
// A plugin might have extended/added some custom type so this
// is a good point to load stuff like components, css rules, etc.

If you want to fix 'default command overriding' I'd suggest you try to move onLoad() at the end of init() (in https://github.com/artf/grapesjs/blob/dev/src/commands/index.js)

@arthuralmeidap
Copy link
Contributor Author

Hi @artf,
Don't worry, I saw the comment but when I ran the tests and everything went fine, I give a try for this PR.
Unfortunately, your suggestion won't work either because the modules are being loaded after the plugins.
One question: Why can't the Command Module load the default commands on init? I have a solution using this approach but one of the tests says : "Main Commands - No default commands on init" and it fails..
Looking forward to hear from you

@artf
Copy link
Member

artf commented Oct 4, 2017

Why can't the Command Module load the default commands on init?

This is exactly what I meant with If you want to fix 'default command overriding' I'd suggest you try to move onLoad() at the end of init() 😄

init() {
    ...
    this.loadDefaultCommands(); // this is all the content from onLoad
    return this;   
}

// Remove onLoad
onLoad() {
    ...

@arthuralmeidap
Copy link
Contributor Author

Hi @artf ,
I did what you suggested. Look and tell what you think about it.
All commands are loaded at the end of Command init and removed toLoad property to not override again the default commands.

@artf
Copy link
Member

artf commented Oct 5, 2017

Great 👍 , can you just bring editor.getModel().loadOnStart(); under the plugin loader then I can merge

@arthuralmeidap
Copy link
Contributor Author

arthuralmeidap commented Oct 5, 2017

@artf Ops, sorry! Fixed now!

@artf artf merged commit d153409 into GrapesJS:dev Oct 5, 2017
@artf
Copy link
Member

artf commented Oct 5, 2017

Thanks Arthur ❤️

@arthuralmeidap arthuralmeidap deleted the fix-custom-plugin-default-command-overriding branch October 5, 2017 11:57
@lock
Copy link

lock bot commented Sep 17, 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 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants