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 to new installation methods #38

Closed
wants to merge 21 commits into from

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented May 27, 2024

This PR migrates demos to use new installation methods (see ckeditor/ckeditor5#15502).

The main change is getting rid of webpack, replacing it with vite, and using ckeditor5 and ckeditor5-premium-features packages only (nightly builds ATM). I took inspiration from https://github.com/ckeditor/new-installation-methods.

Updated demos

There are few demos which cannot be migrated now due to having 3rd-party plugins which do not support NIM yet. Those are:

  • feature-rich (using WProofreader)
  • mathtype (using MathType)
  • productivity-pack (using WProofreader)
  • source-code-editing (using WProofreader)
  • user-interface-document (using WProofreader)
  • wproofreader (using WProofreader)

There was one also demo with custom code where making TS happy was a bit tricky:

  • user-interface-bottom-toolbar

Other minor changes

I made few other minor changes in demo packages itself:

  • Migrated to TS.
  • Moved ckeditor5* packages as devDeps.
  • Removed .main field from package.json (I don't think it's needed for our usage).
  • Improved code comments and missing key messages (based on what @Reinmar intended in 95a8287).
  • In few demos, made additional integrations (like CKBox) to be optional if it's not the main feature to be shown (so that demo does not crash if there is no token), see e.g. here.

Differences in the code

There were some minor changes I noticed when switching to new packages. Not sure if related directly to NIM, switching to TS or demos were a bit outdated. Still noted it down, might be useful to check if any of these could be a part of migration guides:

  • Before, UploadAdapter was imported and used as plugin. Now, it is TS interface and specific implementation needs to be used, e.g. CKFinderUploadAdapter or CloudServicesUploadAdapter for Easy Image (at least that's my understanding).
  • Editor config { "content": ... } is now { translations: lang } (where import lang from 'ckeditor5/translations/ja.js';).
  • Changes in import names, cloudServices are now CloudServices, DocumentList -> List, DocumentListProperites -> ListProperites

Tests

I slightly reworked how tests are run to support old webpack builds (not yet migrated demos) and NIM. For webpack it stayed the same. For NIM, all demos are build and then copied to single tmp builds directory. This way http-server could be run there and cypress can quickly visit and validate each demo (the same way it is done for webpack builds). See 94e193d.

Test run longer now, because I also added missing demos which were not tested before. (Not really, it was Circle CI maintenance yesterday which made runs longer).

CKEditor 5 demos webpage

Most likely https://ckeditor.com/ckeditor-5/demo/ are not build directly from code from this repo, but I'm waiting for confirmation. Confirmed that this repo is only used as public code samples linked from CKE5 demo page and not used to built it.

@f1ames f1ames force-pushed the new-installation-methods-v2 branch 3 times, most recently from ec529a5 to 9fb007c Compare May 28, 2024 13:25
}

// AI Assistant requires additional configuration.
// See https://ckeditor.com/docs/ckeditor5/40.2.0/features/ai-assistant/ai-assistant-integration.html#integrating-with-the-proxy-endpoint
Copy link
Member

Choose a reason for hiding this comment

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

Why link to 40.2.0's docs? If it ws this way in the docs, it's probably some outdated link and should use "latest"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Fixed in cfbf7d4.

@Reinmar
Copy link
Member

Reinmar commented May 28, 2024

There are few demos which cannot be migrated now due to having 3rd-party plugins which do not support NIM yet. Those are:

I think we need to move forward with all of them anyway and just comment those bits out for now. That's what I did in the feature-rich initially.

The biggest problem is the wproofreader demo :| Because it makes no sense without WSC :D Maybe we could keep it migrated on another branch and once we have WSC's release confirmed, merge it to the main one? In other words: to be ready to complete the migration.

@f1ames
Copy link
Contributor Author

f1ames commented May 29, 2024

I think we need to move forward with all of them anyway and just comment those bits out for now. That's what I did in the feature-rich initially.

The biggest problem is the wproofreader demo :| Because it makes no sense without WSC :D

Yes, I guess both WProofreader and Mathtype demos doesn't make sense without those external plugins. For the rest I can migrate commenting out 3rd-party plugins.

Or 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.

Maybe we could keep it migrated on another branch and once we have WSC's release confirmed, merge it to the main one? In other words: to be ready to complete the migration.

Sure, that would work 👍 The question from me is if we want to merge this branch to master now? Because https://ckeditor.com/ckeditor-5/demo links directly to master files so unless NIM is officially released it might be slightly confusing for some user (that we showing NIM as already official installation way)? Or not? WDYT?

@f1ames f1ames force-pushed the new-installation-methods-v2 branch from 9df8fc9 to f82d61e Compare May 29, 2024 07:42
@f1ames f1ames marked this pull request as ready for review May 29, 2024 07:54
@f1ames f1ames changed the title Migrate demos to new installation methods [WIP] Migrate demos to new installation methods May 29, 2024
@f1ames
Copy link
Contributor Author

f1ames commented May 29, 2024

I'm putting this on review and rest of demos (with external plugins), I'll cover in separate branch as mentioned:

Or 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.

I'll keep each demo update as separate commit as here, so we can cherry-pick if needed - Done in #40.

@f1ames f1ames force-pushed the new-installation-methods-v2 branch from cfbf7d4 to 7b7dbd9 Compare May 29, 2024 12:56
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.

All looks great 🎉 but after recent decision of changing the css file names please change all imports from:

import 'ckeditor5/index.css';
import 'ckeditor5-premium-features/index.css';

into

import 'ckeditor5/ckeditor5.css';
import 'ckeditor5-premium-features/ckeditor5-premium-features.css';

headless/index.tsx Outdated Show resolved Hide resolved
Co-authored-by: Piotr Szczęśniak <[email protected]>
@f1ames
Copy link
Contributor Author

f1ames commented Jun 7, 2024

All looks great 🎉 but after recent decision of changing the css file names please change all imports from:

import 'ckeditor5/index.css';
import 'ckeditor5-premium-features/index.css';

into

import 'ckeditor5/ckeditor5.css';
import 'ckeditor5-premium-features/ckeditor5-premium-features.css';

@pszczesniak There is a follow-up PR (covering 3rd-party extensions) - #40, where this had been fixed for all samples. I think we would merge them into one soon (or sync) and then you could maybe just review #40 as this will be redundant.

@pszczesniak
Copy link
Contributor

I think we would merge them into one soon (or sync) and then you could maybe just review #40 as this will be redundant.

@f1ames Yes, that's makes sense 👍

@f1ames
Copy link
Contributor Author

f1ames commented Jun 10, 2024

Closing in favor of #40, all the work will be concluded there.

@f1ames f1ames closed this Jun 10, 2024
@f1ames f1ames deleted the new-installation-methods-v2 branch June 26, 2024 09:29
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.

3 participants