T/3: Highlight UI Implementation #6
Conversation
…hted text of the same value.
…side existing highlight.
|
Switching to another highlight removes the previous one when one decides to continue typing: |
| ```js | ||
| ClassicEditor | ||
| .create( document.querySelector( '#editor' ), { | ||
| highlight: { |
There was a problem hiding this comment.
Since the highlight feature dropdown uses a basic ToolbarView, I wonder if we could simply make the highlight configuration compatible with the toolbar config. I mean here a support for toolbar separators (|), that developers may want to use to group the buttons if only to separate pens from markers. WDYT?
There was a problem hiding this comment.
Very interesting idea. How would the config need to look then?
There was a problem hiding this comment.
Mixed-up with the options:
ClassicEditor
.create( document.querySelector( '#snippet-custom-highlight-options' ), {
toolbar: {
items: [
'headings', '|', 'highlightDropdown', 'bulletedList', 'numberedList', 'undo', 'redo'
],
viewportTopOffset: 60
},
highlight: {
options: [
{ model: 'greenMarker', class: 'marker-green', ... },
{ model: 'bluePen', class: 'pen-blue', ... },
'|',
{ model: 'fooPen', class: 'pen-foo', ... }
'|',
'removeHighlight'
]
}
} )
.then( editor => {
window.editor = editor;
} )
.catch( err => {
console.error( err.stack );
} );Or as a separate property, which makes more sense to me because it doesn't mix up the content logic and the UI.
In such situation, the order in options doesn't matter. We also provide a default toolbar layout, when no specified (and the default is toolbar: [ all options, '|', 'removeHighlight' ])
ClassicEditor
.create( document.querySelector( '#snippet-custom-highlight-options' ), {
toolbar: {
items: [
'headings', '|', 'highlightDropdown', 'bulletedList', 'numberedList', 'undo', 'redo'
],
viewportTopOffset: 60
},
highlight: {
options: [
{ model: 'greenMarker', class: 'marker-green', ... },
{ model: 'bluePen', class: 'pen-blue', ... },
{ model: 'fooPen', class: 'pen-foo', ... }
],
toolbar: [
'highlight:greenMarker',
'highlight:bluePen',
'|',
'highlight:fooPen',
'|',
'removeHighlight'
]
}
} )
.then( editor => {
window.editor = editor;
} )
.catch( err => {
console.error( err.stack );
} );There was a problem hiding this comment.
@oleq AFAIR that was an idea to have UI configurable using button instances (toolbarDropdown) and it should have issue for that. It is also doable the same way for alignment.
There was a problem hiding this comment.
@oleq & @Reinmar : https://github.com/ckeditor/ckeditor5-ui/issues/322 & https://github.com/ckeditor/ckeditor5-heading/issues/74#issuecomment-305224952.
The way how alignment & highlight features works (buttons + dropdown) was inspired by that issue & comment - so thus enables it pretty straightforward.
There was a problem hiding this comment.
ps> I'd for doing this as a follow up just to see this feature live ;)
There was a problem hiding this comment.
ps> I'd for doing this as a follow up just to see this feature live ;)
👍
There was a problem hiding this comment.
But I don't think that https://github.com/ckeditor/ckeditor5-ui/issues/322 has anything to do with this. Here, in this specific feature, there's more than just wrapping buttons into a dropdown, am I right? The state of the button, the color of the pen, etc, has some custom bindings.
|
The question is why in #6 (comment) the selection is changed from collapsed at the end of a highlight to a non-collapsed? |
|
I was sure that it was implemented regarding to this: https://github.com/ckeditor/ckeditor5-highlight/issues/3#issuecomment-352456606 but probably I've wrongly remembered how that should work. So I'll fix that as described in above comment. |
I initially thought that this is the expected behaviour. And that's why https://github.com/ckeditor/ckeditor5-link/issues/73 requires a special treatment. But then I realised that it works differently in CKEditor 4. Please report it as a general ticket in ckeditor5-typing because we need a research. |
I mean – I may be wrong. Do you see something in Olek's comment which would indicate that the selection should change? |
To be precise: I've read (remember) how the selection should work with highlight wrong. Or I've mixed examples from collapsed selection with those from with expanded. It was my mistake. I've fixed it to match how it was described in linked comment. |
src/highlightui.js
Outdated
| const t = this.editor.t; | ||
|
|
||
| return { | ||
| 'Marker': t( 'Marker' ), |
There was a problem hiding this comment.
Why is the yellow marker just "Marker" while the green one is "Green marker"? This is inconsistent and potentially confusing for the user with visual impairments.
docs/features/highlight.md
Outdated
| The `value` corresponds to the `model` property in configuration object. For the default configuration: | ||
| ```js | ||
| highlight.options = [ | ||
| { model: 'marker', class: 'marker', title: 'Marker', color: '#ffff66', type: 'marker' }, |
There was a problem hiding this comment.
Why is this marker named using a different convention than the others?
|
|
||
| The `value` corresponds to the `model` property in configuration object. For the default configuration: | ||
| ```js | ||
| highlight.options = [ |
There was a problem hiding this comment.
I think we need to work on the colors because AFAIR the has been chosen without any deeper research.
Suggested merge commit message (convention)
Feature: Implement highlight feature UI and UX. Closes ckeditor/ckeditor5#2595. Closes ckeditor/ckeditor5#2598.
Additional information