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

t/1: Implemented the first version of the CKEditor 5 component for Angular 2+ #2

Merged
merged 27 commits into from
Jul 16, 2018

Conversation

oleq
Copy link
Member

@oleq oleq commented May 25, 2018

Feature: Implemented the first version of the CKEditor 5 component for Angular 2+. Closes #1.

@ma2ciek
Copy link
Contributor

ma2ciek commented May 25, 2018

I've tested a little bit that package alone and as a dependency. Here are my observations:

  • It doesn't work with the Angular@6 - I'll test it more and I'll write another post about it.
  • The whole package seems to work with Angular@5 as an alone package and as a dependency (but I had to downgrade all deps manually in the package.json to get compatible versions with Angular@5).
  • We can improve types in many places. e.g. EventEmitters and callbacks can be typed more accurate, some properties miss types while others don't need to be explicitly typed. E.g. @Input() data: string = ''; .
  • "strict": true option in tsconfig.json is a common practice, it foreces strong typings and explicit any types and would require type few untyped things and add simple module declarations for ckeditor5 builds.
  • typings.d.ts file isn't used anywhere, but it might be useful for the above declaration file.
  • onInit is imported in the https://github.com/ckeditor/ckeditor5-angular/pull/2/files#diff-78a0ef7a1c99c3df83f45a56085b143e, but it isn't used actually.
  • There's an e2e test in the package.json, which is broken (protractor.conf.js file is missing).
  • Some mixed tabs and spaces in TS files.

@ma2ciek
Copy link
Contributor

ma2ciek commented May 25, 2018

With the build.options.preserveSymlinks: true in the angular.json (in Angular@6) it works fine, so the ckeditor5-angular package works as a dependency in the Angular@6 ecosystem.

@ma2ciek
Copy link
Contributor

ma2ciek commented May 25, 2018

The upgrade to v6 shouldn't be hard, but the version is pretty new (~2 weeks) and I remember how we ended up upgrading webpack to v4. We don't have requests for the newest versions and that package works as a bridge between env that use Angular@6 and CKE5 components, so I don't find upgrading necessary.

Ref: https://blog.angular.io/version-6-of-angular-now-available-cc56b0efa7a4

@nicke
Copy link

nicke commented Jun 12, 2018

I've got a strange effect. The tooltip translations of the toolbar are mixed up.

I'm using angular 5.2.0 with a custom ckeditor5 build. In the sample/index.html of my custom everything is ok.

e.g. in german translation list button with link tooltip
bildschirmfoto 2018-06-12 um 14 01 43

@nicke
Copy link

nicke commented Jun 12, 2018

... i removed the other translations from my custom build (comment out 'additionalLanguages: 'all'') and it's correct for the current language

@Reinmar
Copy link
Member

Reinmar commented Jun 12, 2018

When additionalLanguages is not set, then the defined language is compiled directly into the source (it's inlined there). So that changes the behaviour significantly and I guess that's what saves you here. Still, we'll need to understand why additionalLanguages doesn't work.

@ma2ciek
Copy link
Contributor

ma2ciek commented Jul 12, 2018

I've got a strange effect. The tooltip translations of the toolbar are mixed up.

Still, we'll need to understand why additionalLanguages doesn't work.

That's because three builds are loaded at the same time. We need to leave only one. Especially now, after merging https://github.com/ckeditor/ckeditor5-utils/pull/249/files#diff-53f341fba063aab14909aa1e9b908df1.

@Reinmar
Copy link
Member

Reinmar commented Jul 12, 2018

The good thing is that the error logged by that code will make it clear what happened :)

@ma2ciek
Copy link
Contributor

ma2ciek commented Jul 13, 2018

I've updated the code, so the current implementation works well in the angular@6 environment.

What works?

  • The CKEditorComponent works with CKEditor5 builds (e.g. with https://github.com/ckeditor/ckeditor5-build-classic) and custom builds in the Angular@6 env.
  • The CKEditorComponent works alone and as a dependency in another project that might have a lower angular version.
  • It should be easily tested using unit tests and e2e tests (the component takes care of the ngZone).
  • There're two simple demos, including one with forms.
  • There're very simple type declarations for the editor builds. They can be improved for sure, but they should land in the ckeditor5's builds. There're missing types for editor events and the editor itself. They could potentially improve the user experience.
  • There's an ng-packagr added by @oleq that allows this package to be easily built and published.

What doesn't work?

  • CKEditor5 can't be built from source in the angular@6 environment. It would require some additional work and I'm not sure if it needs to wait on the ng eject which is still in progress for angular@6. But I've heard that new CLI tool is more flexible, so wh knows? For now, there is at least the SVG loader collision.

  • Two different editors in the application. Now, you neither can build editor from the source in an angular project to have two different editors nor include two different builds (it blows up translations and now it's forbidden, see https://github.com/ckeditor/ckeditor5-utils/blob/master/src/version.js#L51). The only way to fix that would be enabling building from source. (Note that you can have many editors of the same type in the application now).

What needs to be improved and considered?

  • Should we add ckeditor5-angular to mgit.json in the CKE5 main repo? AFAIR it didn't work well with lerna and adding angular ecosystem results in even bigger node_modules and longer lerna bootstrap times.
  • CI for this package
  • I'm not sure about package.json's dependencies. How much we can move to dev dependencies and peer dependencies and how much should stay in dependencies.
  • ng add support
  • ng eject when it will be available
  • some more advanced unit tests to show how this component should be tested (?)

@jodator
Copy link

jodator commented Jul 14, 2018

Should we add ckeditor5-angular to mgit.json in the CKE5 main repo? AFAIR it didn't work well with lerna and adding angular ecosystem results in even bigger node_modules and longer lerna bootstrap times.

Noooooo... IMHO adding angular to the CKE5 would be overkill.

@ma2ciek ma2ciek merged commit 61e0f6e into master Jul 16, 2018
@ma2ciek
Copy link
Contributor

ma2ciek commented Jul 16, 2018

I'll keep that branch for a while, as users might still use the old branch.

Copy link

@Guellord26 Guellord26 left a comment

Choose a reason for hiding this comment

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

'pok'
ok

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.

6 participants