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

Document colors section for font/background color dropdowns #2286

Closed
mlewand opened this issue Mar 29, 2019 · 22 comments · Fixed by ckeditor/ckeditor5-font#39
Closed

Document colors section for font/background color dropdowns #2286

mlewand opened this issue Mar 29, 2019 · 22 comments · Fixed by ckeditor/ckeditor5-font#39
Assignees
Labels
package:font type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@mlewand
Copy link
Contributor

mlewand commented Mar 29, 2019

Is this a bug report or feature request?

🆕 Feature request

💻 Version of CKEditor

CKEditor v5 @ 12.1.0

📋 Steps to reproduce

The document colors section of a dropdown should pick up colors present in the editor contents at a time of opening the dialog.

This means that it will simply contain unique colors used throughout the document. The colors should appear in the same order as in the document (starting from top). Also the colors should be unique within the sections (they can duplicate colors present in the generic dropdown).

Config

There should be a config property allowing to customize the number of items in document colors section or allowing to disable it by setting it's value to 0. FontColorConfig#documentColors will do.

ClassicEditor
	.create( {
		fontColor: {
			documentColors : 0 // Disable document colors section.
		}
	} )

📃 Other details that might be useful

We initially wanted to add this feature in 12.1.0 but ended up in extracting it to this dedicated issue and pushing it back for future release.

Edit: Down the discussion you might see references to "recent colors" features, which is a different features that we considered - but ultimately we proceeded with document colors feature.

@mlewand
Copy link
Contributor Author

mlewand commented Mar 29, 2019

I wanted to report paste handling as a separate ticket, but this would actually be handled by this particular ticket.

@mlewand
Copy link
Contributor Author

mlewand commented Mar 29, 2019

One important aspect to think about here is collaboration.

Should colors used by other users be propagated to local user's dropdown?

The color is applied to the document, so logically it should be "consumed" and displayed by the recent colors. I'm only afraid that it could lead to the situation where local user will get frustrated because he or she will lose recent colors as other users paste/use different colors.

Please share your opinions.

@scofalik
Copy link
Contributor

scofalik commented Mar 29, 2019

If you are interested in colors that were in the editor after the data was loaded, you could set a listener on Editor#event:ready (or maybe .once() on model.Document#event:change but I am not sure that would work ;)).

In that listener, you can simply scan the whole model (make a range on root and get through .getItems()) and check all returned text proxies if they have an attribute.

You could do that every time dropdown is opened to get the exact data all the time but this may be not effective in case of long documents.

If you want to dynamically update the list (pasting, collaboration) you would have to listen to model.Document#event:change and use editor.model.document.differ.getChanges() to get the changes. Then scan insertions (deeply) and attribute changes to add new colors to "recent colors" section.

The one problem with this solution is that it is more complicated to dynamically remove colors because you would have to count the occurrences of each color. You can check that a text with given color was removed but you don't know if that was the last occurrence of a text with that color in the document. @oleq gave a good example of someone pasting a text with multiple colors which would bloat/overwrite recent color section and then executing undo.

A nice thing about having this working with collaboration is that you and other collaborators won't start coloring stuff with similar but different colors.

@Reinmar
Copy link
Member

Reinmar commented Apr 2, 2019

If you are interested in colors that were in the editor after the data was loaded, you could set a listener on Editor#event:ready (or maybe .once() on model.Document#event:change but I am not sure that would work ;)).

We're interested in the colors which are in the document when the panel is being opened. So it's a no-brainer.

@Reinmar
Copy link
Member

Reinmar commented Apr 2, 2019

Quoting myself from https://github.com/ckeditor/ckeditor5-font/issues/32

There are 2 colors used in the document so they should be visible in the "document colors" section in each panel.

Currently, on the colors which I applied during the current editing session will appear there. Whcih means that I won't be able to reuse colors which were pasted or which someone else applied or which I applied in the previous session.

image

@mlewand
Copy link
Contributor Author

mlewand commented Apr 15, 2019

There's couple of design decisions that we need to clarify before moving forward.

One thing that has been brought up recently is the way how exactly the recent colors are picked. There are two strategies:

Please share your opinions here or on a F2F meeting.

Other matters to consider:

  • Recent colors behavior with collaboration? Should colors used by other users also affect recent colors set?
  • Should the amount of recently used colors be configurable (including opting out the feature)?

@mlewand
Copy link
Contributor Author

mlewand commented Apr 15, 2019

My take on strategy is that the approach of loading colors upon each dropdown open provides very little use for the end user. Assuming you have used multiple colors in the document, and you're doing something at the end of the document, it's possible that you won't see your color at all.

Having the colors added in order that they actually were used seems logical to me. If the performance is the problem I think we should do a research to determine how could we adjust/provide the API to make it in a performance-friendly way. For sure similar cases will occur in the future.

@scofalik
Copy link
Contributor

scofalik commented Apr 15, 2019

Recent colors behavior with collaboration? Should colors used by other users also affect recent colors set?

I personally think this is a very good UX. No problem with reusing "that exact gray" the other person used. Depending on the solution this will be either very difficult or "free".

Should the amount of recently used colors be configurable (including opting out the feature)?

Why not? We could set some default limit and have a config option to change it.

It's possible that you won't see your color at all.

With limiting, yes, I agree, good point.

@jodator
Copy link
Contributor

jodator commented Apr 15, 2019

I see this boils down to a recent vs document colors.

For me recent colors are colors that I've chosen from the Dropdown. So:

  • only my colors are there as I use the app
  • might be empty on first load.

The other cases described are document colors:

  • picking up colors from document
  • collaboration colors (picked up from document).

So I think that either recent colors do too much or are named inaccurately...

Opting-out from this could be added.

I personally think this is a very good UX. No problem with reusing "that exact gray" the other person used.

I'd see format painter to be used there.

@oleq
Copy link
Member

oleq commented Apr 15, 2019

One thing that has been brought up recently is the way how exactly the recent colors are picked. There are two strategies:

  • Currently the colors are picked up continuously as content is modified. This means that recent colors are added to the set in order they were used, but it affects performance as it requires computing on each diff. #28 (comment)
  • Other option would be to do "lazy load" of recent colors, so that the colors are only loaded when the color dropdown is open. This means that the colors are displayed in order of the document. (#28 (comment))

A hybrid idea: Can we load document colors once (let's say when drop–down first shows up) and then push user (recent) colors up–front as they use the dropdown to colorize the content?

If the performance is an issue, let's stick to the scenario in which only direct usage of the color in the dropdown adds to the list.

  • Recent colors behavior with collaboration? Should colors used by other users also affect recent colors set?

It makes sense to me. But again, if this is a performance issue, we do not need that so badly to sacrifice the UX.

  • Should the amount of recently used colors be configurable (including opting out the feature)?

👍

@Reinmar
Copy link
Member

Reinmar commented Apr 15, 2019

  • Other option would be to do "lazy load" of recent colors, so that the colors are only loaded when the color dropdown is open. This means that the colors are displayed in order of the document.

I'm for this solution (and calling it "document colors") because:

  • it's simple (cheap to code)
  • it's cheap performance-wise
  • it works with collaboration (you can see all peoples' colors)
  • it works on fresh editor load automatically too
  • unless you have 5+ different colors in the document, you won't even see a difference
  • we can change it in the future if we get feedback that it doesn't work

@mlewand
Copy link
Contributor Author

mlewand commented Apr 17, 2019

I can see that the feature is going to be cheaper in terms of development. But I don't think that users will find this useful/predictable.

Anyway, we need to go with something and since the other option would take us a much more time to ship then be it, I'll have to 🙈 on this one. Let's continue with document colors feature.

I have updated the issue summary.

UX

BTW should we put a label before this colors section? cc @oleq @dkonopka

Config

I was thinking of a simple config name for document colors feature and it should be as simple as: documentColors.

We shouldn't go with documentColorsCount as it's even longer and prevents us from extending it down the road.

Extensibility-wise though we'll use numbers today, there's nothing holding us back to make it an object in future if there will ever be a need to, say:

ClassicEditor
	.create( {
		fontColor: {
			documentColors: {
				label: false,
				count: 12,
				isVisibble: () => this.colors.count() >= 3
			}
		},
		fontBackgroundColor: {
			documentColors: 0 // Disable document colors section.
		}
	} )

@mlewand mlewand changed the title Font/background color should pick up colors used in the document Document colors section for font/background color dropdowns Apr 17, 2019
@oleq
Copy link
Member

oleq commented Apr 17, 2019

BTW should we put a label before this colors section? cc @oleq @dkonopka

I'd need to see it in the mockups to tell. Probably yes.

@mlewand
Copy link
Contributor Author

mlewand commented Apr 17, 2019

I'd need to see it in the mockups to tell. Probably yes.

@dkonopka can I ask you to put a small mockup for this one?

@dkonopka
Copy link
Contributor

IMHO we should definitely use a label here (I've already pushed it to the t/28 branch).

Screen Shot 2019-04-23 at 10 37 02

@mlewand
Copy link
Contributor Author

mlewand commented Apr 23, 2019

Thanks for the design @dkonopka. I wonder whether the spacing might be changed so that there's less margin below the "Document colors" label (top margin looks fine). It would reduce the height and make "Document colors" appear to be visually more related to the colors section below.

@dkonopka
Copy link
Contributor

@mlewand What is interesting, there is no bottom/top padding for this label, it's just all based on line-height of this div. Changing this value looks for me like a little CSS hack, but it's not a no-go.

Screenshot 2019-04-24 at 10 24 25

Changing line-height of label adding padding-top

.ck.ck-color-grid__label {
  /* We need to overwrite line-height calulated via `.ck-reset`. */
  line-height: 1em;

  padding: var(--ck-spacing-standard) var(--ck-spacing-standard) 0;
}

Screenshot 2019-04-24 at 10 26 32

Current state

Screenshot 2019-04-24 at 10 28 00

@oleq WDYT?

@msamsel
Copy link
Contributor

msamsel commented Apr 24, 2019

FYI: @Reinmar, @mlewand
Approach with document colors can also have some flaws.
When document won't contain colors, then we will have to iterate over entire model.

@mlewand
Copy link
Contributor Author

mlewand commented Apr 25, 2019

When document won't contain colors, then we will have to iterate over entire model.

@msamsel searching entire model for a given attribute should not be such an inefficient operation. So I don't think we should be concerned about this case.

Thanks for refreshed designs @dkonopka. Since @oleq is off for couple of days. @msamsel please proceed with initial proposal. We'll resolve the alignment later on.

@msamsel
Copy link
Contributor

msamsel commented May 15, 2019

There appeared bad behaviour of UI, where currently selected color had double selection in dropdown panel. This happens, when user use default color in text, so both color grids had selected the same color.
sample

After consulting with @mlewand & @dkonopka, we decide to remain selection only in Document Colors, if this feature is available.

@mlewand
Copy link
Contributor Author

mlewand commented Jul 3, 2019

This feature recently was merged to the master branch 🎉

The only thing you need to do is enable the Font plugin, as it's document colors section is enabled by default there.

More info will be available in the docs.

Sample screencast:
Screencast showing new document colors being added to the dropdown as end user adds more colors

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-font Oct 8, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:font labels Oct 8, 2019
@mlewand mlewand added this to the iteration 25 milestone Oct 8, 2019
@mahindragithub
Copy link

How to Change HSL to RGB font color

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:font type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants