-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Implement the Highlight feature #2597
Comments
I don't think we can make this optional. It would not make sense, because the class name is anyway required (see bellow).
Just as a note to remember... the highlight command should expect the class name of the marker to be applied. The title cannot be used because it must be localizable.
Hum... the only other way I see is sniffing the CSS to retrieve the color from it. It is a much nicer way, but is it worth the additional code?
What about making it a configuration property instead, just to make it look simpler? Btw, as we're in the crazy CSS sniffing thing, we could even sniff this property as well :) |
The engine implementation of this feature seems to be very simple. The complex part is the UI. Here we can even thinking about separate milestones, if you wish. |
I mean in minimalistic configuration it would be just ClassicEditor
.create( {
highlight: {
markers: [ { class: '' }] // I can already see what's not nice here
}
} ); so it would have only one, default marker - maybe it not make sense but that was initial idea.
I don't see from where we could sniff that option. It would require some
Yep, I'd go with engine part in this ticket and the UI in follow up.
So in other words configuration should be: const configA = {
highlight: [
{ class: 'marker-important', title: 'Important', color: '#555', type: 'marker' },
{ class: 'pen-typo', title: 'Typo', color: '#555', type: 'pen' }
]
};
// OR
const configB = {
highlight: [
{ class: 'marker-important', title: 'Important', background: '#555' },
{ class: 'pen-typo', title: 'Typo', foreground: '#555' }
]
}; |
Shouldn't this feature use model markers and highlighting it in the view as @scofalik mentioned it in #631 (comment)? |
@szymonkups & @scofalik That is a good question for you guys actually as you have more experience with engine. The output of Highlight feature will be: <p>
This is some text <mark class="marker-important">that is cool</mark>.
</p> Give above it looks for me as regular |
@szymonkups & @scofalik please don't overcomplicate this features... it is exactly like the bold feature, so please give a nice answer to @jodator that will not make him crazy :) |
I would KISS in the first version. This can be brought later if we'll ever need to support it. |
Well, we would have to be creative... like creating a hidden element with the classes applied and checking their styles or anything like that.
Yes, ofc... I was just opening the possibility if we want to make it more complex. But this is the kind of thing that can land later if we want. |
😄 If we treat this feature the same as for example Bold feature then this is fine. Model markers and highlighting mechanism could be used when there will be a need for overlapping or image highlighting. I brought this up just to be sure that we are purposefully want to implement this without using model markers. 👌 |
What I've meant was that we do not have external (content) CSS stylesheet to apply those classes. How to sniff is rather simple and doable. As for now we lack that |
It depends on what you want to achieve. Markers are ranges. If you have, for example:
and paste "xyz" inside "ipsum" you will get... With marker:
Without marker:
Same, if you cut & paste ipsum, you will get... With marker:
Without marker:
BTW. It's easier to operate on markers if you want to, for example, remove the whole style, or remove all styles at given position. It's also easier to find all occurrences of given style. BTW2. I still hate the fact that there will be "invisible" spans in the final output and that somebody may have see them in the source. |
If we use, model markers it will be always one consistent range. Whatever happens, it will not split. If we use attributes it will be attached to text, like bold. It will be possible to move part of the highlighted text. In this case, I believe, it should work like bold feature, as @fredck mentioned to make it simpler. Markers are designed for different cases, where you need to be able to find or change marked fragment easily. They are more CPU-consuming since they need to recalculate with each change, and does not work well with undo. They are designed to work well with external logic which will handle them, like comments or external users selection. They are not created to just mark text, for such case attributes should be used. |
Feature: Initial implementation of highlight editing. Closes #1.
This issue is a follow up of talks that have taken:
The feature should be developed in the dependency injection–friendly way (see example):
Model and data
<mark>
element with optional class.UI
It's a TODO because ATM there's no such possibility. Things to consider:
Things to consider:
The text was updated successfully, but these errors were encountered: