Docs: Multiple root editor sample and tutorial added #1586
Conversation
Docs: Updated Babel's packages in Advanced setup guide. Closes #1553.
…ing. Aligned wording in the UI section of the guide to the previous parts.
|
Wow🎉 It works and looks great. From my side I see that some wording will need to be polished, but nothing big. Great job :) |
|
Reviewed from my side. Leaving the source code to @oleq. |
|
|
| // it isn't), e.g. by setting the proper CSS class, visually announcing focus to the user. | ||
| // Doing otherwise will result in editable focus styles disappearing, once e.g. the | ||
| // toolbar gets focused. | ||
| editable.bind( 'isFocused' ).to( this.focusTracker ); |
There was a problem hiding this comment.
Why all roots are marked as focused when just one has focus at a time? This is not the right UX.
The relation with the FT should be different in this multi–root editor:
- if any editable is focused ->
FT#isFocused === true - if an editable is focused, only that editable gets focused styles as per
EditableUIView
There was a problem hiding this comment.
We had some discussion with @pjasiun some time ago about this case - https://github.com/ckeditor/ckeditor5-editor-decoupled/issues/21#issuecomment-448943315 (the outcome was that it is acceptable). However, if it can be easily changed with the current API, I'm in favor of making this change to have only one root marked as focused at the same time.
There was a problem hiding this comment.
If you want to change it, that's fine, but TBH I like it this way. It stresses the fact that this is a single editor.
| // it isn't), e.g. by setting the proper CSS class, visually announcing focus to the user. | ||
| // Doing otherwise will result in editable focus styles disappearing, once e.g. the | ||
| // toolbar gets focused. | ||
| editable.bind( 'isFocused' ).to( this.focusTracker ); |
There was a problem hiding this comment.
See my comment in MultirootEditorUI#init(). All editables cannot share the same isFocused state.
| editingView.attachDomRoot( editableElement, editable.name ); | ||
| } | ||
|
|
||
| this._initPlaceholder(); |
There was a problem hiding this comment.
How does it work? I assume the same placeholder text is set to all roots, which does not make sense in most cases IMO. It would take editor.config.placeholder defined as { root: 'Root placeholder text', ... } to make this right.
There was a problem hiding this comment.
I assume the same placeholder text is set to all roots, which does not make sense in most cases IMO.
Yes, this is the way it works in this implementation. It would make much more sense to use it with { root: 'Root placeholder text', ... } as you mentioned, however it will require some changes in placeholder code (probably mostly docs?). I assume we could cover this as a follow-up?
Docs: Added two new guides 🎉.
|
I'm going to merge this because it actually works (with some minor issues). Let's fix the rest on |
Suggested merge commit message (convention)
Docs: Multiple root editor sample and tutorial added. Closes #1447.
Additional information
See #1447 and https://github.com/ckeditor/ckeditor5-editor-decoupled/issues/21.
There are two things I'm not 100% sure about. The naming of the editor type which is Multiple root editor - generally it makes sense for us, but not sure how easy to find it will be for the users/developers interested in such editor type. I took a look on other editors and usually it is something like shared toolbar, but it covers a little different cases.
The other thing is the content of the sample itself, I used the content from Inline build sample, because it has four containers so works nicely with multiple root editor and can be used to see what is the difference between multiple root editor and the approach using multiple editor instances. My doubt is that it may be confusing that we have two samples with the same content.