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

Update grapesjs compatibility (Mautic 5 + CKE5) #51

Closed

Conversation

LordRembo
Copy link
Contributor

This PR is specific for the ckeditor5 branch, so we can use the updated preset in 5.x

Registering your custom components using the legacy method breaks when updating GrapesJS to release 0.20.1. Need this to be able to upgrade GrapesJS in the GrapesJSBuilderBundle.

Related PR's:

@LordRembo
Copy link
Contributor Author

Hmm, I found an issue when loading GrapesJS, that traces back to the Editorfonts service from the preset. Gonna have to investigate a bit.

@LordRembo
Copy link
Contributor Author

Okay, so it's not directly related to the changes in this PR but to updates in GrapesJS, combined with the CKE5 stuff.
Essentially, the Editorfonts service is missing a check for the typography property, to avoid the JS breaking when firing before the Editor's Stylemanager and typography property exist. I'll add a check and have it throw a proper message instead.

Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is this PR ready to review? I'm not sure based on your last comment.
  2. If it is, it looks like most of the changes are mostly code style changes. Were those changes done by a script?
  3. What changes were done by hand? Those outside of the dist directory?
  4. Is there a way to test these changes?

@LordRembo
Copy link
Contributor Author

@escopecz

  1. I just finished reviewing the issue, turns out it's nothing related to my changes, so we're good to go
  2. The code changes outside the dist folder were done by me, the rest is done by running the build script
  3. Basically, some syntax that was already deprecated becomes invalid when updating to GrapesJs 0.21.1
  4. The way to test this, is to
  • have a Mautic 5.x running locally
  • replace the package with my forked branch in plugins/GrapesJsBuilderBundle, eg. by running npm i grapesjs-preset-mautic@github:LordRembo/grapesjs-preset-mautic#update-grapesjs-compat-after-cke5 inside that bundle
  • rebuilding GrapesJs bundle by running npm run build
  • Make or edit an email and see that you can still use drag & drop to add/move/edit a Dynamic content Block.
  • you should still be able to save that edited mail

@escopecz
Copy link
Member

Cool!

Another question is why we need #49. Can we just merge this into the 5.x branch?

@LordRembo
Copy link
Contributor Author

Ah, yeah, I made this PR before the ckeditor5 branch got merged. I'll close PR #49

@LordRembo
Copy link
Contributor Author

No, it's the other way around. This PR has to be closed because it is targetting the cke5 branch that was already merged. The changes are now for #49

@LordRembo LordRembo closed this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants