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

Migrate demos with external plugins to new installation methods #40

Merged
merged 38 commits into from
Jun 26, 2024

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented May 29, 2024

This PR migrates demos with 3rd-party plugins to NIM. Follow-up of #38:

I can prepare separate branch migrating all demos with external plugins which will be waiting to be merged. So we have still all demos fully functional (and fully featured) on master and then will be ready when those external plugins are migrated.

It's based on new-installation-methods-v2 branch (#38 PR) and I followed all the same conventions and assumptions. You can read #38 description for all details.

The main difference here is that 3rd-party plugins (WProofreader and Mathtype) are commented out in CKEditor 5 plugins list in it's config in each demo. This way demos can be run and after plugins are migrated to NIM, it should be enough to just uncomment those lines.

@f1ames f1ames force-pushed the new-installation-methods-3rd-parties branch 3 times, most recently from 18e6ae2 to 88a523b Compare May 29, 2024 12:53
@f1ames f1ames force-pushed the new-installation-methods-v2 branch from cfbf7d4 to 7b7dbd9 Compare May 29, 2024 12:56
@f1ames f1ames force-pushed the new-installation-methods-3rd-parties branch from 88a523b to c4d95b3 Compare May 29, 2024 13:02
@f1ames f1ames marked this pull request as ready for review May 29, 2024 13:18
@f1ames f1ames changed the title Migrate demos with external plugins to new installation methods [WIP] Migrate demos with external plugins to new installation methods May 29, 2024
@f1ames f1ames requested a review from Reinmar May 29, 2024 13:19
'selectAll',
'wproofreader',
'|',
// 'insertTemplate',
Copy link
Member

Choose a reason for hiding this comment

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

I've just tripped on this. I provided the license key, as it asked me to, but then surprise, surprise: those premium buttons weren't there. I had quite a big WTH moment :D 

The good thing about our toolbar initialization is that even if you used a button that wasn't registered, the editor will work and you will get a warning.

So it's not necessary to comment out missing buttons. And it's definitely confusing :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍 I'll fix it - unless you already did 👀

Co-authored-by: Piotr Szczęśniak <szczesniakp@o2.pl>
@f1ames
Copy link
Contributor Author

f1ames commented Jun 7, 2024

Why did you rewrite the demos to TS?

My worry here: So far, we keep our resources in JS (docs and most other samples I can think of).

It was a part of this task as we initially discussed with @Witoso and @arkflpc. But I don't know if there is a longer story behind this, maybe they can give more context.

8f66b1f is a bit ugly commit but I tried to figure out why the demos don't work

Turned out there were two reasons: dup modules because of WSC and the change in CSS file names that we did recently.

Yeah, I pushed it before the CSS file names changes, also @pszczesniak pointed that out in the first PR - #38 (review). I see you already updated it 🙇 👍

I started pushing here changes that we have to do to make the cke5-demos repo follow changes that were implemented on ckeditor.com.

@Reinmar maybe let me know what you are working on here, and how cna I help so we don't duplicate the work.

@f1ames f1ames force-pushed the new-installation-methods-3rd-parties branch from 96e56a0 to 7461409 Compare June 10, 2024 14:06
@f1ames
Copy link
Contributor Author

f1ames commented Jun 11, 2024

Ready for review.

The only thing missing is enabling MathType and WProofReader in couple of demos, but this needs to wait for NIM compatible versions of those plugins.

@f1ames f1ames requested a review from pszczesniak June 11, 2024 09:38
@Reinmar Reinmar changed the base branch from new-installation-methods-v2 to master June 12, 2024 08:34
headless/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@pszczesniak pszczesniak left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@f1ames
Copy link
Contributor Author

f1ames commented Jun 13, 2024

Ready to merge, but I suggest to wait till MathType and WProofReader are bumped to NIM compatible versions?

@pszczesniak
Copy link
Contributor

@f1ames i've tested WProofreader with alpha version "@webspellchecker/wproofreader-ckeditor5": "3.0.0-alpha.0" and it works ok.

@arkflpc arkflpc merged commit 6fd0142 into master Jun 26, 2024
1 check passed
@arkflpc arkflpc deleted the new-installation-methods-3rd-parties branch June 26, 2024 09:25
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.

4 participants