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

Highlight UI & UX #2595

Closed
dkonopka opened this issue Nov 13, 2017 · 97 comments · Fixed by ckeditor/ckeditor5-highlight#6
Closed

Highlight UI & UX #2595

dkonopka opened this issue Nov 13, 2017 · 97 comments · Fixed by ckeditor/ckeditor5-highlight#6
Labels
domain:accessibility This issue reports an accessibility problem. package:highlight status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@dkonopka
Copy link
Contributor

This is a follow up of #631.

In proposals below, you can see two types of presenting color attributes.

I'm definitely for v2 (color grid ) with a label because we need to explain users difference between "pen" and "markers", in my opinion, it's not clear to everyone (I mean icons). Version one is like the old-school style, so I'm not sure about it.

Additionally, we can use "Highlight color" & "Highlight background" to be different from old tools naming like "Font color/Text color/Font background".

highlight

@jodator
Copy link
Contributor

jodator commented Nov 13, 2017

I like the third option - the one without text - the most with fourth/fifth as second.

Maybe if marker/pen icons could be a bit more distinct from each other? Just a though as currently they are pretty similar (especially when scaled down a bit). Also marker tip in icon has narrower tip then in pen icon.

@Reinmar
Copy link
Member

Reinmar commented Nov 13, 2017

Maybe if marker/pen icons could be a bit more distinct from each other? Just a though as currently they are pretty similar (especially when scaled down a bit). Also marker tip in icon has narrower tip then in pen icon.

I actually wanted to comment that I have no idea what these icons mean. I can only guess that one of them sets the text color, the other bg color, but I don't know what the 3rd option does.

EDIT: After having 3rd look at this I noticed the label there :D It makes it more clear now and perhaps will be sufficient for the users. But I'd still agree with @jodator that icons could be more distinctive.

@fredck
Copy link
Contributor

fredck commented Nov 13, 2017

I had a dream once about this feature. I would love to see how that would look like :)

@fredck
Copy link
Contributor

fredck commented Nov 13, 2017

Btw, we can also think well about the list of colors we want to configure by default.

@Reinmar
Copy link
Member

Reinmar commented Nov 13, 2017

Btw, we can also think well about the list of colors we want to configure by default.

Let's use some set proposed by Stabilo ;)

image

@fredck
Copy link
Contributor

fredck commented Nov 13, 2017

I don't think that more than four colors is necessary for the scope of the feature. This is configurable, after all.

@fredck
Copy link
Contributor

fredck commented Nov 13, 2017

I'm for this:

  • Markers: Yellow, Orange, Bluish, Redish
  • Pens: Red, Blue, Green

@jodator
Copy link
Contributor

jodator commented Nov 13, 2017

@Reinmar or maybe from Crayola? 😉

@fredck
Copy link
Contributor

fredck commented Nov 13, 2017

Nah... maybe the "basic" Pantone set then:

image

:trollface:

@dkonopka
Copy link
Contributor Author

dkonopka commented Nov 14, 2017

Updated proposals then:

Should "eraser" button remove background & color of highlight or we need separate buttons? In my opinion, we should stay with one (KISS).

Another question: what is more important if color&background is enabled, which icon should we show? Both as proposed?

highlight

Color & background highlight enabled:

screen shot 2017-11-14 at 11 01 04

@jodator
Copy link
Contributor

jodator commented Nov 14, 2017

@dkonopka no mixing (for sure in MVP) so it's either background or color at once.

@fredck
Copy link
Contributor

fredck commented Nov 14, 2017

Let's get back to the starting point here to not mess things up.

We wanted to propose this feature based on a specific use case: users want to mark parts of the content with some color (usually as a review tool).

Let's pay attention to not transform this feature into the old "Foreground and Background Color" feature. It's about "highlight" and one can do so either by using a marker or by writing in a different color. That's our goal.

@fredck
Copy link
Contributor

fredck commented Nov 14, 2017

I believe that the option I click in the panel should be the one I see in the toolbar after that. I mean, the panel should list icons that are also used as the toolbar button icon.

@fredck
Copy link
Contributor

fredck commented Nov 14, 2017

Btw, "Yellow Marker" seems to be the most common option for this (it's, in fact, the way browsers style <mark> by default). Therefore, maybe we should have first markers and then pens in the panel, with the yellow marker being the first one.

@dkonopka
Copy link
Contributor Author

dkonopka commented Nov 15, 2017

I'm still not sure if the user will properly recognize marker and pen icon (as you @fredck suggested in #631 (comment) ). I was trying to put there even A/T letter to show typography, but it's not fit as good as I was expecting.

Proposal 2&3 clearly represent a state of the upcoming action. In my opinion, it doesn't look like old-type background/foreground tool and there is no way to wrongly understand event "behind" icon.

1. Highlight background and Highlight color as pen&marker icon

screen shot 2017-11-15 at 10 02 26

2. Highlight with common marker icon

screen shot 2017-11-15 at 10 01 01

3. Highlight with common cropped marker icon

screen shot 2017-11-15 at 10 01 07

@jodator
Copy link
Contributor

jodator commented Nov 15, 2017

Cropped icon looks better. I have only problem with too small contrast on those icons (2&3) - the "zigzag" icons were better in that matter as there was better contrast.

@fredck
Copy link
Contributor

fredck commented Nov 15, 2017

Another idea is making the options in the panel different than the icon, but also bigger. That was my initial idea in the mockup I've put down on paper earlier.

And sincerely, the proposed options don't look good to me. For now, the 3rd option proposed here is the one I like the most: https://github.com/ckeditor/ckeditor5-highlight/issues/3#issuecomment-344206677

@dkonopka
Copy link
Contributor Author

Another idea is making the options in the panel different than the icon, but also bigger

I was thinking about it too, do we really need to "copy" icon from the main toolbar to options panel?

the "zigzag" icons were better

☝️ I really prefer "zigzags" more than "marker/pen" icons. I think the first proposal looks modern opposite to default "color-palette" in most apps. @oleq WDYT?

1. Inline "zigzags"

screen shot 2017-11-15 at 10 53 33

2. "Zigzags" with label

screen shot 2017-11-15 at 10 59 04

3. "Color-palette" style

screen shot 2017-11-15 at 11 01 23

@oleq
Copy link
Member

oleq commented Nov 15, 2017

TBH, to me, the best option is "3. Highlight with common cropped marker icon"

image

I just looked at it and I immediately knew what the feature is about, what to expect and how to use it.

Ideas

  • Maybe splitting the palette into two rows (first one for the bg markers, second one–for the text markers, no text labels) would make it even more useful (easier to read, and ready for 20 markers). Where to put the eraser then?
  • Maybe let's separate (a simple gap would do the trick) the bg markers from the text markers a little bit but keep them it a single line?

Issues

Although I like the icons, please note that people may create blue/black/dark markers. They must be visible in the palette, which has a dark background and vice versa if someone used a bright theme for the editor. They must be bulletproof.

What we need in the mockups:

  • Examples of bright (yellow) and dark (blue) text and bg markers when the palette is dark (like in the mockups above),
  • Examples of bright (yellow) and dark (blue) text and bg markers when the palette is bright.

It's all about contrast and basic usability and accessibility. We mustn't allow any icon to disappear in any situation.

@pjasiun
Copy link

pjasiun commented Nov 15, 2017

I like "Color-palette" style" very much. This clean and simple.

I don't get what "highlight color" is. Is it text color? Can we call it "highlight color" and "font color" like MS Office do?

@jodator
Copy link
Contributor

jodator commented Nov 15, 2017

I'll add this one to have more options as I asked @dkonopka to create this variant (but he didn't like it as much as zigzag ;) ). Probably the line on the bottom is to be removed in this version:

screen_shot_2017-11-15_at_10 36 01_720

@dkonopka
Copy link
Contributor Author

dkonopka commented Nov 16, 2017

@oleq With gaps it looks more separated and very clear. Like we talked about contrast and case with black/dark green color/marker, we will wait for community feedback.

Anyway, if someone want so much black marker then it will be possible to change balloon background (or even icon?). I think is edge case when user will need dark marker similar to our balloon bg.

screen shot 2017-11-16 at 16 30 25

@jodator
Copy link
Contributor

jodator commented Nov 22, 2017

Alrighty guys ( @oleq & @dkonopka ) do we have this UI settled down? I'm ready to implement the dropdown panel as I've already bootstrapped SplitButton in ckeditor5-ui so I'd love to hear if this is final (at least at this stage) or do you need more work?

@Reinmar
Copy link
Member

Reinmar commented Nov 22, 2017

So, do I understand correctly that the last proposed version is this one?

highlight feature design

I like its simplicity, but at the same time it worries me that these icons will not be understandable. I got confused by this feature initially.

So, if we'd go this way, we'll need to work hard on proper icons. Also, labels will be important to get it right.

But we can also address this issue (the lack of clarity) by choosing a more verbose version like this one:

image

I really like it and it and so did @pjasiun. It's clear and simple and easier to extend.

@dkonopka
Copy link
Contributor Author

IMO it's a final design as we talked with @oleq. Ready for comments about details like icons/spacings, but yes, it's the last version of Highlight mockup.

@fredck
Copy link
Contributor

fredck commented Nov 22, 2017

We're aware that these icons have strong accessibility issues, right? Even trying to push it with the "dark yellow" used in the design, the contrast with the white pen inside of it is not good. Imagine what would happen if we put the real yellow there.

Additionally, a real-life marker is a pen with a colored tip... exactly what we use to show a "pen" in the proposed design, not the marker.

@jodator
Copy link
Contributor

jodator commented Nov 22, 2017

@fredck that was a reason I've pushed this mockup (a dark marker/pen). But taking into a consideration that in split button should also represent last used marker (ie "yellow background") it shold be distinguishable between marker/pen variants. In other words the "more verbose" version is less verbose then that.

In my opinion the zygzags are pretty close to:

  • differentiate background/font color icon
  • will not attach it's meaning to background/font color too much as I get a feeling that having "Highlight background" and "Highlight color" would eventually drop it's semantics from this feature and will be turned into font/background color..

@fredck
Copy link
Contributor

fredck commented Nov 22, 2017

will not attach it's meaning to background/font color too much as I get a feeling that having "Highlight background" and "Highlight color" would eventually drop it's semantics from this feature and will be turned into font/background color..

Strongly agree on this.

@wwalc
Copy link
Member

wwalc commented Nov 23, 2017

What I'm afraid of is that we have a dark toolbar and the background color of the wysiwyg area will be usually white. Which means that usually:

  • dark fonts will be used for writing (black & all variants of dark gray)
  • bright colors will be used for highlight background, to have good enough contrast with highlighted dark text
  • highlight colors may be again rather "darkish" (due to points above)... and I'm worried about that. For example, for me the proposed yellow font is hardly readable on a white background. The colors used in mockups are not quite realistic, because we need something with reasonable contrast on a white background.

If you look at the proposed colors in CKEditor 4 some time ago:
screen shot 2017-11-23 at 09 53 43
You will see that with most of the proposed designs, e.g.
screen shot 2017-11-23 at 10 08 44
there is a chance that it may be really hard to really notice some of the darker colors included there.

@scofalik
Copy link
Contributor

scofalik commented Nov 23, 2017

My 5cts. I think that the less "graphical" the icons, the better. I like most the version without any additional icon (3. "Color-palette" style), but zig-zags are okay too. Markers icon are too overwhelming, totally against the whole "lightweight" UI.

Please, guys, don't go with marker-icons, they... don't look good. Trust me on this :P

@oleq
Copy link
Member

oleq commented Dec 19, 2017

An idea of colorful, switchable icons that reflect current style:

Marker

icons-01b-marker

Pen

icons-01b-pen

@dkonopka
Copy link
Contributor Author

dkonopka commented Dec 19, 2017

I really like proposed icons, they are fresh & modern. But in my opinion for the current toolbar it is not fitting as much as it could (for this icon set).

If we are going to refresh UI for CK5 these new icons could look very good IMHO, for dark-theme too.

@Reinmar
Copy link
Member

Reinmar commented Dec 20, 2017

I'd wait for the rest of the theme to catch up. These are really nice icons themselves but they don't match the rest of the icons because they are very graphical (the first one is even comic-like).

@oleq
Copy link
Member

oleq commented Dec 21, 2017

To unblock @jodator I pushed the icons in ckeditor/ckeditor5-highlight@3523d5f. They might not be perfect but they do the trick. The inner color can be adjusted using CSS

svg .ck-icon__fill { fill: yellow }

I think the easiest way to do that at this moment is to retrieve path.ck-icon__fill from the features buttons' icons and apply inline styles to them with JS.

kapture 2017-12-21 at 12 08 35

@jodator
Copy link
Contributor

jodator commented Jan 17, 2018

The color of the icon’s line will come from the same stylesheet where colors used in the content are specified (especially that one needs to specify colors to be displayed in the dropdown’s panel anyway). In other words: content in editable, styles in the dropdown and the line should share the same CSS classes (right, @Reinmar?).

@oleq: Some previous talks required one to define this color in configuration:

editor.config.define( 'highlight', [
	{
		model: 'marker',
		view: { name: 'mark', class: 'marker' },
		title: 'Marker',
		color: '#ffff66',
		type: 'marker'
	}
] );

The color value is currently used as color set to buttons in toolbar and dropdown. Probably instead of using this config value I can set the view configuration on buttons and it should work as long as the page defines those styles like this:

.marker {
    color: #ffff66
}

I'd like to clarify the solution (either):

  1. using color from config.
  2. using view definition on buttons (would require copying class, style, attributes if set (as it might use ViewElementDefinition here) but no need to define color in config.

@Reinmar
Copy link
Member

Reinmar commented Jan 17, 2018

The current config is too verbose but for a different reason. We don't need to define that it's the marker attribute – this should be hardcoded.

As for the color... I'm for defining it in the config. Retrieving it from the stylesheet by using the same class names will be really tricky IMO. Those styles would be then hard to write (e.g. you couldn't use the mark element name there). And it would be even more tricky if more attributes are used.

Finally, the option name should be highlight.options. We may need to set more config options for the highlight feature so we should not take the entire namespace.

@jodator
Copy link
Contributor

jodator commented Jan 17, 2018

@Reinmar: Another question: should highlight behave the same as basic commands on collapsed selection when not in highlight? Ie.: apply highlight to a collapsed selection and start typing - the text will be highlighted?

@Reinmar
Copy link
Member

Reinmar commented Jan 17, 2018

I'm not 100% sure now. Do you remember @oleq what we decided?

@oleq
Copy link
Member

oleq commented Jan 18, 2018

I'm not sure if we discussed this behavior, but I assume highlight is a post-creation tool. That would mean such an action (highlight collapsed selection, then type) should not be possible. But what about the UI then? Should the toolbar icon be disabled in a collapsed selection?

@jodator
Copy link
Contributor

jodator commented Jan 18, 2018

To help you think on this I've created a screen cast with such behavior:
screencast 2018-01-18 10_28_19

First thought: a bit flashy toolbar button :D

@jodator
Copy link
Contributor

jodator commented Jan 18, 2018

But looks OK to me after more time fiddling with this.

@jodator
Copy link
Contributor

jodator commented Jan 18, 2018

ps.: wouldn't be better to unselect highlighted text and create collapsed selection to range end?

It is not visible ATM that selection get highlighted (nothing changes) as selection cover highlight.

Other features' changes are visible under selection (like bold, italics, font-size, etc).

@oleq
Copy link
Member

oleq commented Jan 18, 2018

Other features' changes are visible under selection (like bold, italics, font-size, etc).

What do you mean by that? Pressing CTRL+B does not change anything (visually) when the selection is collapsed.

@jodator
Copy link
Contributor

jodator commented Jan 18, 2018

@oleq I've talking about other case on expanded selection (normal use-case). When you select some text and execute highlight - visually nothing happens. While executing bold the text get bolded and it is visible under a selection. Similar with italic. But for highlight you have to unselect the text to actually see command's result on text.

@oleq
Copy link
Member

oleq commented Jan 18, 2018

Now I get it. Maybe we can tune the styles for (at least) markers so the native selection does not cover the entire highlighted area?

@jodator
Copy link
Contributor

jodator commented Jan 18, 2018

👍 for markers, but 👎 for pens:
selection_021

@jodator
Copy link
Contributor

jodator commented Jan 18, 2018

::selection {
	background: rgba(0, 145, 255, 0.8);
}
mark {
	padding-bottom: 2px;
}

With above (on Chrome):
selection_022

@jodator
Copy link
Contributor

jodator commented Jan 18, 2018

The same as above on FF:
selection_024

@oleq
Copy link
Member

oleq commented Jan 18, 2018

Doesn't look too bad to me. I'd wait for more feedback, though.

@Reinmar
Copy link
Member

Reinmar commented Jan 18, 2018

  1. Let's go with the standard behaviour of the bold/italic buttons for collapsed selection. We can easily change it (e.g. to apply the highlight to the entire word) later. It will be easier to make the decision once we see and use it live and going with the familiar behaviour should be safe for now.
  2. The button needs to be on when the selection is inside a highlight. This is an important visual feedback that the feature reacts to your actions.
  3. Regarding the selection styles – nice catch. However, we can easily tweak this over time, so let's not spend too much time on this now. Let's have a followup for this and avoid overriding the ::selection completely because it's not safe either.
  4. We realised that there's a problem with leaving the highlight at the end of the paragraph (just like with the link feature). It's a significant issue because the "remove highlight" button removes the entire highlight, not just the style of the selection (which would allow escaping the style). So, we'll need to generalise the link's feature to all styles perhaps. But let's have a followup for this too and not block the highlight feature.

@oleq
Copy link
Member

oleq commented Jan 18, 2018

Sounds like a plan. As for the link–at–the–end–of–the–line issue, I'm leaving this for future reference: https://github.com/ckeditor/ckeditor5-link/issues/72#issuecomment-352458361. Can't wait to see it stable!

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:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:highlight labels Oct 9, 2019
@Reinmar Reinmar added the domain:accessibility This issue reports an accessibility problem. label Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:accessibility This issue reports an accessibility problem. package:highlight status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants