-
-
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
Document colors section for font/background color dropdowns #2286
Comments
I wanted to report paste handling as a separate ticket, but this would actually be handled by this particular ticket. |
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. |
If you are interested in colors that were in the editor after the data was loaded, you could set a listener on In that listener, you can simply scan the whole model (make a range on root and get through 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 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. |
We're interested in the colors which are in the document when the panel is being opened. So it's a no-brainer. |
Quoting myself from https://github.com/ckeditor/ckeditor5-font/issues/32
|
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:
|
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. |
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".
Why not? We could set some default limit and have a config option to change it.
With limiting, yes, I agree, good point. |
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:
The other cases described are document colors:
So I think that either recent colors do too much or are named inaccurately... Opting-out from this could be added.
I'd see format painter to be used there. |
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.
It makes sense to me. But again, if this is a performance issue, we do not need that so badly to sacrifice the UX.
👍 |
I'm for this solution (and calling it "document colors") because:
|
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. UXBTW should we put a label before this colors section? cc @oleq @dkonopka ConfigI was thinking of a simple config name for document colors feature and it should be as simple as: We shouldn't go with 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.
}
} ) |
@dkonopka can I ask you to put a small mockup for this one? |
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. |
@mlewand What is interesting, there is no bottom/top padding for this label, it's just all based on 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;
} Current state@oleq WDYT? |
@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. |
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. After consulting with @mlewand & @dkonopka, we decide to remain selection only in Document Colors, if this feature is available. |
How to Change HSL to RGB font color |
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.📃 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.
The text was updated successfully, but these errors were encountered: