Switch to CKEditor #394

vladikoff · 2017-11-16T00:26:07Z

TODO

This commit sets up the classic ckeditor to be used in place of quill. Most functionality (specifically with the footer buttons and syncing) do not work as of yet.

The two footer buttons: `enable-sync` and `give-feedback` now work as intended. When `enable-sync` is clicked however, the `sync-note` covers the footer buttons currently instead of being displayed above them.

Formats added: - All text sizes - block quotes Also added the `cursor: text` to the text editor area.

This was an attempt to build the Classic editor from source as outlined in this guide: https://docs.ckeditor.com/ckeditor5/latest/builds/guide- s/integration/advanced-setup.html. This isn't bundled correctly however since ClassicEditor.js and Markdown.js are at the top-level of the directory and need to be inside the `sidebar/` directory.

vladikoff · 2017-11-16T14:07:54Z

Hello @szymonkups and @Reinmar

This part is going mostly well but could use your advice on the following:

change text direction button + RTL support, do we need to write a plugin?
theming the editor for a dark theme (I think we can do it by just CSSing), but would like to hear your thoughts
do we need to write a plugin for strikethrough?

vladikoff · 2017-11-16T14:59:59Z

src/sidebar/panel.js

-      });
-    }
-  });
+  // content.forEach(node => {


@szymonkups, @Reinmar Need a bit of help here. We used to look at node attributes in Quill to detect of features are being used. How do we detect if bold, italic, etc are used? getData just returns text. In Quill there is quill.getContents(); vs quill.getData();

It'd be something like:

import Range from '@ckeditor/ckeditor5-engine/src/model/range'; const range = Range.createIn( editor.document.getRoot() ); for ( const value of range ) { if ( value.item.is( 'text' ) ) { console.log( Array.from( value.item.getAttributes() ) ); } }

Iterating over a range uses a tree walker and value is an instance of TreeWalkerValue.

This will print which text attributes are used. You can also check which elements are used by console.log( value.item.name ) if value.item.is( 'element' ) is true.

Of course, this means that you need to build this code together with CKEditor. So you can't use a built version of CKEditor, but build CKEditor from source together with the code which uses it. It's explained here: https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/framework/guides/quick-start.html.

We're now working on making it simpler to write some basic features without building CKEditor. This means exposing more APIs somewhere in the editor properties.

PS. Why do you need to check which features are used in the editor?

PS. Why do you need to check which features are used in the editor?

So we can learn which features are the most popular. User research, etc.. :)

@Natim wanna take a look at this?

need to uncomment the content.forEach(node => { function, otherwise metrics works.

"uncomment" => make it work. You might need to add a dependency that includes the Range module.

build CKEditor from source together with the code which uses it

@Natim Our build script is here: https://github.com/mozilla/notes/pull/394/files#diff-bef1bc0d0ce8255123c3de896c5a796b , builds and copies with npm run postinstall

It doesn't seems to work:

ERROR in ./src/ckeditor.js Module not found: Error: Can't resolve '@ckeditor/ckeditor5-editor-classic/src/classiceditor' in '/home/rhubscher/mozilla/notes/ckeditor-build/src' @ ./src/ckeditor.js 6:0-85

Trying to install npm install --save @ckeditor/ckeditor5-editor-classic

npm install --save @ckeditor/ckeditor5-build-classic nailed it.

Reinmar · 2017-11-16T15:01:55Z

Great to see that you're moving forward with this :) I really want to see the final result.

The bidi feature (similar to CKEditor 4's bidi plugin) is something that you'd need to implement. I guess that we'll implement it one day ourselves, but it's not on our roadmap yet. We could help you to start it. And as for UI's support for RTL, this is something that you could do yourself too, especially that I see that you're hacking the theme anyway :). RTLed UI is on our roadmap, but I don't know when we'll work on it.
See https://docs.ckeditor.com/ckeditor5/latest/framework/guides/ui/theme-customization.html. But if you can wait one month, then it'd be better because we're now switching to PostCSS which will make it easier. You can read more here: 🏁 Release: CKEditor 5 v1.0.0 second alpha ckeditor/ckeditor5-design#182 in the "UI improvements" section.
Yep, you'd need to write it, but it will be very simple. Just look at examples in https://github.com/ckeditor/ckeditor5-basic-styles/tree/master/src.

vladikoff · 2017-11-16T17:03:51Z

Migration:

render quill hidden if you have notes in storage.
quill.container.firstChild.innerHTML get the html
call setData with html. Delete the notes storage key
use notesMarkdown for the new notes storage key
add metrics event for migration
only load quill if we have to do a migration
( * backup to localstorage and use that for backup maybe)

vladikoff · 2017-11-16T18:22:41Z

@Reinmar thank you so much for the quick response. It is really helpful!

@cedricium I pushed fixes to the dark theme, it's ~70% there. Let me know if you have time to tweak the styles-dark.css. I won't be making changes to it for now.

cedricium · 2017-11-16T19:24:49Z

@vladikoff sure thing, will work on the dark theme 👍

cedricium · 2017-11-17T20:44:22Z

@vladikoff so far I have a fix for the 'Choose heading' label overflow and I made the link color more visible for the Dark theme. Is there anything else you saw that needs specific fixing?

Changes made result in the following: - fixed the heading dropdown border overlap issue - replicated `:hover` and `:active` styles from the light theme - added a border to tooltips to make them stand out better

Dark Theme touchups

Natim · 2017-11-20T10:12:42Z

Early Beta to start QA: addon-1.9.0b1.zip

…or-concept

…nto ckeditor-concept

vladikoff · 2017-11-20T18:43:35Z

@cedricium awesome work on the dark theme 👍

Fixes #410

Fixes #413 Fixes "Hello!" is not a heading on fresh start"

* Fix hardcoded version number * Simplify build script and npm start

Fixes #408

vladikoff · 2017-11-21T02:57:16Z

@Natim this should be ready for release. Could you go over the code and see if you can see anything we should adjust?

One final thing to check is the metrics. Let me know if you got time to check that before merging.

I couldn't get these to work:

~~Missing placeholder~~ Waiting on Implement configurable editor's placeholder ckeditor/ckeditor5#479
~~Focus on open~~ Delayed until The content area from "Firefox Notes" sidebar does not gain focus when it is opened or switched #330

but I think we can ship without that. I removed the ~~strike~~ shortcut for now. Hopefully does not block us

vladikoff · 2017-11-21T02:58:00Z

@cedricium feel free to also take a look ^

cedricium · 2017-11-21T03:38:51Z

-- Disregard, got it working. --

vladikoff · 2017-11-21T03:59:39Z

-- Disregard, got it working. --

Awesome :)

Natim · 2017-11-21T09:50:14Z

addon-1.9.0b2.zip

Natim · 2017-11-21T09:54:42Z

src/background.js

@@ -10,7 +10,7 @@ const analytics = new TestPilotGA({
  ds: 'addon',
  an: 'Notes Experiment',
  aid: 'notes@mozilla.com',
-  av: '1.9.0dev'  // XXX: Change version on release
+  av: browser.runtime.getManifest().version


Natim · 2017-11-21T09:56:05Z

I am going to land this to give more time to translators to catch up before next week release.

vladikoff · 2017-11-27T17:11:28Z

@szymonkups and @Reinmar,

this is now live: https://medium.com/firefox-test-pilot/testpilot-notes-v1-9-0-b166c6cd7353
Thanks so much for your help!!

cedricium and others added 6 commits November 2, 2017 11:34

Initial implementation of the ckeditor for Notes

ad83da0

This commit sets up the classic ckeditor to be used in place of quill. Most functionality (specifically with the footer buttons and syncing) do not work as of yet.

Removed commented lines regarding Quill

b172ba4

Enabled footer buttons functionality

3cf3a30

The two footer buttons: `enable-sync` and `give-feedback` now work as intended. When `enable-sync` is clicked however, the `sync-note` covers the footer buttons currently instead of being displayed above them.

Added additional formats to be used by ckeditor

d7d0c01

Formats added: - All text sizes - block quotes Also added the `cursor: text` to the text editor area.

Clean up and custom build

5d458ed

vladikoff mentioned this pull request Nov 16, 2017

Ckeditor concept #382

Closed

vladikoff added 2 commits November 15, 2017 20:44

More editor cleanup, fix up css

f741d04

clean up link balloon a bit

eac7966

Update note logic for ckeditor

540d03f

vladikoff commented Nov 16, 2017

View reviewed changes

Make dark theme almost work

39f8c30

vladikoff and others added 7 commits November 16, 2017 15:43

Add migration script, fix Welcome text

099c2b9

Add missing dependencies.

397b9cb

Add getPadStats for CKEditor.

3b90966

Fix travis config.

ee5445e

Add ENV variables to give context.

9468be3

Fix linter errors.

8f05954

Add strike plugin

fd35054

cedricium and others added 3 commits November 17, 2017 21:08

Dark Theme touchups

af4ee36

Changes made result in the following: - fixed the heading dropdown border overlap issue - replicated `:hover` and `:active` styles from the light theme - added a border to tooltips to make them stand out better

Merge pull request #399 from cedricium/ckeditor-concept

cf24be2

Dark Theme touchups

Use P rather than 14.

a628561

cedricium and others added 3 commits November 20, 2017 09:18

Merge branch 'master' of https://github.com/mozilla/notes into ckedit…

7b9c1d6

…or-concept

Merge branch 'ckeditor-concept' of https://github.com/mozilla/notes i…

1f56f65

…nto ckeditor-concept

Fix text cursor, update deps

b798eb5

vladikoff added 13 commits November 20, 2017 14:03

Fix right click on toolbars, Fix tooltips

8445529

Fixes #410

Add comment for fixed bug

b0d6c29

Fix typo

5df24a1

Patch out heading until it is fixed upstream

911b86b

Merge branch 'master' into ckeditor-concept

46921a1

Use new strike icon

ff5ce1b

Add ordered list styles, add heading

fdacfce

Fixes #413 Fixes "Hello!" is not a heading on fresh start"

Update deps, remove extra focs

1794eaa

Fit icons into single row with the default sidebar width

f9cd3af

Remove extra console statement

b599f90

CSS Tweaks

38d6060

* Fix button localization

5b8b640

* Fix hardcoded version number * Simplify build script and npm start

Fix too much space between rows

911fb87

Fixes #408

vladikoff mentioned this pull request Nov 21, 2017

Text size is not changed to 14, if the sidebar contains paragraphs of different sizes including 14 #386

Closed

Natim reviewed Nov 21, 2017

View reviewed changes

Natim merged commit d3a0b7e into master Nov 21, 2017

pdehaan mentioned this pull request Dec 28, 2017

Remove references to Quill? #533

Closed

Natim deleted the ckeditor-concept branch February 15, 2018 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to CKEditor #394

Switch to CKEditor #394

vladikoff commented Nov 16, 2017 •

edited

Loading

vladikoff commented Nov 16, 2017 •

edited by Natim

Loading

vladikoff Nov 16, 2017

Reinmar Nov 16, 2017

vladikoff Nov 16, 2017

vladikoff Nov 17, 2017

vladikoff Nov 17, 2017

vladikoff Nov 17, 2017

Natim Nov 17, 2017

Natim Nov 17, 2017

Natim Nov 17, 2017

Reinmar commented Nov 16, 2017

vladikoff commented Nov 16, 2017

vladikoff commented Nov 16, 2017

cedricium commented Nov 16, 2017

cedricium commented Nov 17, 2017 •

edited

Loading

Natim commented Nov 20, 2017

vladikoff commented Nov 20, 2017

vladikoff commented Nov 21, 2017

vladikoff commented Nov 21, 2017

cedricium commented Nov 21, 2017 •

edited

Loading

vladikoff commented Nov 21, 2017

Natim commented Nov 21, 2017

Natim Nov 21, 2017

Natim commented Nov 21, 2017

vladikoff commented Nov 27, 2017

Switch to CKEditor #394

Switch to CKEditor #394

Conversation

vladikoff commented Nov 16, 2017 • edited Loading

TODO

vladikoff commented Nov 16, 2017 • edited by Natim Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Nov 16, 2017

vladikoff commented Nov 16, 2017

vladikoff commented Nov 16, 2017

cedricium commented Nov 16, 2017

cedricium commented Nov 17, 2017 • edited Loading

Natim commented Nov 20, 2017

vladikoff commented Nov 20, 2017

vladikoff commented Nov 21, 2017

vladikoff commented Nov 21, 2017

cedricium commented Nov 21, 2017 • edited Loading

vladikoff commented Nov 21, 2017

Natim commented Nov 21, 2017

Choose a reason for hiding this comment

Natim commented Nov 21, 2017

vladikoff commented Nov 27, 2017

vladikoff commented Nov 16, 2017 •

edited

Loading

vladikoff commented Nov 16, 2017 •

edited by Natim

Loading

cedricium commented Nov 17, 2017 •

edited

Loading

cedricium commented Nov 21, 2017 •

edited

Loading