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

Define the what the highlight feature is #2598

Closed
Reinmar opened this issue Nov 27, 2017 · 11 comments · Fixed by ckeditor/ckeditor5-highlight#6
Closed

Define the what the highlight feature is #2598

Reinmar opened this issue Nov 27, 2017 · 11 comments · Fixed by ckeditor/ckeditor5-highlight#6
Labels
package:highlight status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Nov 27, 2017

Related tickets: https://github.com/ckeditor/ckeditor5-highlight/issues/3, #631.

We've had a chat with @oleq and @wwalc about the direction this feature needs to take because it got blurry with time. Summary on what we agreed on (plus some open stuff):

  1. "Text highlighting" is meant to be a feature solving a set of use cases like marking text during reviews, for future reference, etc. We've all been there and we know the Stabilo markers so I hope this is clear.

  2. There's the <marker> tag which is designed exactly for this purpose and we're going to use it. What's more – we're going to use classes not styles so to make this feature as semantical as possible.

  3. Font color and background color features which we know from every visual editor in the word are separate, independent features. The existence of the highlight feature doesn't block us from implementing them in the future. Furthermore, we can see use cases for fg/bg color features which the highlight feature should not solve (e.g. purely visual content editing, matching paste from Word styles, etc.).

  4. In real life, people use fg/bg color features as a substitute for the highlight feature but that doesn't mean that the highlight feature doesn't make sense. People, given the freedom, use text color for all kind of stuff from quoting (vide Outlook), through emphasizing a piece of content (instead of using italic/bold), to coloring the content so it matches some styleguide or one's esthethics.

  5. To make it as clear for people as possible what the highlight feature is all about (especially, if we expect to provide the other two features too), we need to define it as precisely as we can. The better we define its use cases, the more substantial and distinctive it will be and, hence, better for its job.

  6. So far, the discussion was about highlight color and highlight background color. This lead to many issues already – the names are rather confusing ("text highlight" == "highlight background color"), the UI got more complex and we couldn't decide how to differentiate these via ideograms. I think that @fredck is right that people may also want to mark text with a different text color but if we'll reflect this visual aspect of this feature in the UI (by giving options to set those colors independently) we'll make it less semantical and more visual, hence, more confusing for developers (and perhaps users too).

    So, I propose to have just a single set of colors. These colors will be mapped to class names, so the developer will be able to style those <mark> elements in whatever way anyway.

    The wins:

    * The UI gets simpler. The icon in the toolbar can represent a marker (highlighter) and in the panel we can have just one set of color boxes and no other label, icon whatever is needed.
    * The feature should be easier to understand for people because (I hope we agree) highlighting will be associated with marking text by changing its bg like we used to do using physical markers.
    * We can always add text (fg) color highlights to this feature in the future if we'll see the need to do so.
    * And we may not need to do so because a single, class-based feature has great flexibility thanks to external stylesheets. We may consider, though, how to make the color boxes in the panel styleable too (e.g. I guess they should have classes), so developers are able to better reflect the variations that they will create. E.g. one could add "A" inside each box using ::before style (or a config option, if we'll provide it) and change its color to reflect a marker which changes text color instead of bg color.

    Edit: see https://github.com/ckeditor/ckeditor5-highlight/issues/4#issuecomment-349045556. We'll support two types of highlights from the beginning as proposed initially.
    The proper names for these two types of highlights are "pens" and "markers".

  7. I think that this wasn't discussed yet, but we need to define how users will be highlighting text in the editor. E.g., taken how you'll use it in real life (which is to mark stuff which is already written), I think that after you chose a highlight color in the panel, clicking inside the editor should mark entire words, not just set the caret in that place. And if you click two subsequent words, the space between them should be included too. So, that's completely different than the basic styles. OTOH, it's a question whether basic styles should not work differently too and whether this is something to work on now.

    Edit: see https://github.com/ckeditor/ckeditor5-highlight/issues/4#issuecomment-349045556:

    the aspect of applying highlights to the text (point 7.) was discussed somewhere already and that, as you all commented here, it should not happen as in "paint mode". Furthermore, the style will be exclusive (can't find a better word), so only one marker can be applied to one character (which is important information since fg and bg styles will be available).

@jodator
Copy link
Contributor

jodator commented Nov 27, 2017

I think that after you chose a highlight color in the panel, clicking inside the editor should mark entire words, not just set the caret in that place

Only when clicking a word? I mean I think that there may be a use case that user would like to highlight only a part of a word.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 27, 2017

Only when clicking a word? I mean I think that there may be a use case that user would like to highlight only a part of a word.

I agree. That should be doable if you select a piece of content and pick a highlight or vice versa – pick a highlight and select a piece of the content. Although, I know that some editors extend the selection to full words when you select content which sometimes is super helpful and sometimes is really irritating. So I don't know how I'd like it to work :D

@scofalik
Copy link
Contributor

My POV.

  1. Keep it simple. A background color is enough. A few colors are enough. We may even want to get inspired by Stabilo and use some of their colors :). Don't make it too complicated because you will start making "Styles" feature out of it (you know the feature where you have a class linked with a label name and you choose them from dropdown... like "fancy box", etc. :)).

  2. The idea that color boxes in UI should have distinctive classes set so a developer can provide styles for them is something that I got too when reading your post, so +1 for that.

  3. You'll never make it really semantic unless the classes are important, note, etc. I mean, class="green" is not much better than style="background: #00AA00;". But it is better and I am of course for using classes. Side note on this: highlight should have a set of up to several styles and use classes. Font color/BG color should have bigger palette but also a possibility to edit color's hex code. This will be more useful when pasting from Word (which will be the primary use case for font/bg color.)

  4. As far as UX is concerned -- with selection, highlight selection. Without selection, highlight first selection done by user. If user pressed a key (ctrl) stay in highlight mode (dunno if doable).

All in all, I don't think we settled the main issue: is this feature meant to keep colors when the content (HTML) is rendered on the website? I know that, all in all, it's developers choice to include CSSes or not. But what are our view on this?

@Reinmar
Copy link
Member Author

Reinmar commented Nov 27, 2017

Ad 1. 👍

Ad 2. 👍

Ad 3. I've been thinking about this too. But I don't think that we'll be able to define proper semantical classes, so we should go with colors. That's not a problem as long as we'll make it configurable (which we of course will). Also, classes are better than styles because they solve your final doubt:

I know that, all in all, it's developers choice to include CSSes or not. But what are our view on this?

That it's up to the developer, so we need to use classes. Also, we don't provide target page's stylesheets (our content styles are merged into the editor's stylesheet), so the default behaviour will be that markers won't be displayed (unless <marker> has user agent styles defined).

@jodator
Copy link
Contributor

jodator commented Nov 27, 2017

All in all, I don't think we settled the main issue: is this feature meant to keep colors when the content (HTML) is rendered on the website?

AFAIR that was discussed earlier - it gonna be mix of <mark> and class="foo" to define a highlighter and this feature would require a developer to define those classes on a webpage that displays content (as well in the editor).

@Reinmar
Copy link
Member Author

Reinmar commented Nov 27, 2017

Don't make it too complicated because you will start making "Styles" feature out of it

BTW, good point about the Styles feature. It was requested as well (and makes sense) so it's the 3rd feature to add to fg/bg color and highlight features ;).

@oleq
Copy link
Member

oleq commented Nov 28, 2017

I think that this wasn't discussed yet, but we need to define how users will be highlighting text in the editor. E.g., taken how you'll use it in real life (which is to mark stuff which is already written), I think that after you chose a highlight color in the panel, clicking inside the editor should mark entire words, not just set the caret in that place. And if you click two subsequent words, the space between them should be included too. So, that's completely different than the basic styles. OTOH, it's a question whether basic styles should not work differently too and whether this is something to work on now.

So what you propose here is basically a painter?

  1. click in the toolbar to enable it,
  2. then whatever you select gets "painted" (marked/highlighted),
  3. click in the toolbar to disable it.

If yes, then I'm afraid this approach has some UX issues:

  1. Discovering that this is a painter could be painful. Users will apply several hightlights before they notice what's going on. That might be really annoying.
  2. "I can't exit vim" syndrome that I mentioned before. It means that users will struggle to leave the painter mode and the struggle will cost them some accidental highlights in their content before they figure that out.
  3. If selecting text applies the highlight, how would balloon toolbar be invoked in such situation? (it shows up upon selection) How to exit the painter mode without applying the highlight? I guess this kind of toolbar will be very popular and I guess we cannot simply say that this feature is incompatible.
  4. Is there any sane strategy to make the feature accessible? I see no easy way to make it work with the screen readers where people discover the content and navigate by moving the selection. Painter feels like a tricky concept for users with severe visual impairments.
  5. I think users are not used to the painter concept in general. It's a UX of the image editing software moved to the text editor and it feels out of place. While it makes sense in some cases like in a style painter (note that the name clearly indicates what type of tool it is), I see no good reason to use it for the highlight feature. I'm aware that the association with a physical writing tool is strong here but I believe we should be consistent in our UX.

I'm for a classical approach instead:

  1. select text,
  2. click in the toolbar to select the highlight color,
  3. text gets highlighted,
  4. select some other text,
  5. click the button in the toolbar – the last used highlight color is applied.

@dkonopka
Copy link
Contributor

dkonopka commented Nov 29, 2017

I'm for a classical approach instead:

👍 x 100.

The idea of a painter is not intuitive for users - I know it is similar to real life use case, but it is too risky to choose that solution, they will freak out really fast if they don't know how to make a simple highlight.

@fredck
Copy link
Contributor

fredck commented Nov 29, 2017

In my opinion, this issue is a DUP, if not invalid. The feature has already been defined in various separate issues and summarized in ckeditor/ckeditor5-highlight#1. Here, in fact, we're duplicating discussions which already happened and were settled earlier, before ckeditor/ckeditor5-highlight#1 was closed.

The engine part of the features is already implemented and closed with ckeditor/ckeditor5-highlight#2. The only missing aspect of this feature is the UI, which is already handled in #2595.

@Reinmar
Copy link
Member Author

Reinmar commented Dec 4, 2017

We've talked with @fredck about the functionality of this feature (point 6. of my initial post) and we're going with two types of highlights (as previously discussed) – text highlight and background highlight (names are not definitive).

The reasoning is that:

  • there are use cases for text highlights which are clearly used in the wild,
  • if we'll go with just the background this feature will look pretty much like a background color feature, so instead of clarifying this feature we'd make it more confusing.

Also, @fredck clarified to me that the aspect of applying highlights to the text (point 7.) was discussed somewhere already and that, as you all commented here, it should not happen as in "paint mode". Furthermore, the style will be exclusive (can't find a better word), so only one marker can be applied to one character (which is important information since fg and bg styles will be available).

I'll update my initial post with this information.

@Reinmar
Copy link
Member Author

Reinmar commented Dec 12, 2017

I added this bit to my initial post:

The proper names for these two types of highlights are "pens" and "markers".

We didn't care to use them in the wireframes so far, but we must not forget them. And in this case, it's better to use the proper names from the beginning because they are a crucial aspect of this feature.

oleq referenced this issue in ckeditor/ckeditor5-highlight Feb 19, 2018
Feature: Implemented the user interface of the highlight feature. Closes #3. Closes #4.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-highlight Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:task This issue reports a chore (non-production change) and other types of "todos". package:highlight labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:highlight status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants