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

Implement the font and background color feature #1457

Closed
oleq opened this issue Jan 14, 2019 · 11 comments
Closed

Implement the font and background color feature #1457

oleq opened this issue Jan 14, 2019 · 11 comments
Assignees
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@oleq
Copy link
Member

oleq commented Jan 14, 2019

Is this a bug report or feature request? (choose one)

🆕 Feature request


The ckeditor5-highlight feature works with a pre-defined (finite, limited) set of markers/pens and works best with structured highlights, for instance, to establish a communication between a content creator and a proofreader in a document.

We've already had some feature requests suggesting that the Highlight feature is unsatisfactory for certain integrations, and a free text/background coloring tool is necessary, offering a full palette of colors without any restrictions, just like the CKEditor 4 feature. This feature would have a different (no) content semantics and a different UI/UX.

image

image

Related:

cc @Reinmar @dkonopka

@oleq oleq added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). status:confirmed labels Jan 14, 2019
@Reinmar Reinmar added this to the backlog milestone Jan 14, 2019
@pjasiun
Copy link

pjasiun commented Jan 14, 2019

Idea: usually you use the limited set o colors. It would be nice if the color pallet suggests you colors you already used or which are already applied in the document.

In fact, this is where highlight and color features are similar: highlight feature might have an option to add a custom "highlights" in the runtime and add automatically colors/highlights which are already applied in the document.

@Reinmar Reinmar mentioned this issue Feb 6, 2019
@msamsel msamsel self-assigned this Mar 4, 2019
@mlewand
Copy link
Contributor

mlewand commented Mar 15, 2019

Just a short summary of the scope for our MVP:

UI

  • In toolbar we're going to use the split button. Down arrow will open the dropdown where more colors might be selected, whereas button's face applies lastly selected color (if no color was yet selected the first one from the color palette gets used) .
  • Dropdown will feature the remove color button, that will essentially remove the style. It's position should be determined during UI design phase.
  • Both foreground and background color will get a separate button.
  • Preferrably: let's not go crazy with the default color palette size.
  • Column count should be figured out automatically based on config.
  • There should be "last color" section, that lists most recently used colors. It should consider recent colors used by the end user, colors pasted from external sources as well as font/bg colors in loaded editor content.
  • Data-wise, there needs to be support for all kind of proper CSS color formats. So hex format, rgb, rgba, hsl etc.

Nice to haves

  • Reflecting the active color on the button icon is to be verified. It might be as well pushed back to implement it later on.
  • It would be nice to give possibility to make a separate groups of colors using the configuration.

Followups not suitable for MVP

  • The "more colors" option - not suitable yet for MVP.

Config example

Simplified config might look something like this:

[
	[ 'ff0000', '0f0', '00f' ],
	'|',
	[ 'f30', 'f30', 'f30', 'f30', 'f30', 'f30', 'f30' ],
	[ 'f30', 'f30', 'f30', 'f30', 'f30' ],
	[ 'f30', 'f30', 'f30', 'f30', 'f30', 'f30', 'f30' ],
	[ 'f30', 'f30', 'f30', 'f30', 'f30' ]
]

There should be also a possibility to specify custom labels.

@mlewand mlewand modified the milestones: backlog, iteration 23 Mar 15, 2019
@dkonopka
Copy link
Contributor

1. Color palette

Choosing default grid with 5 colors in a row, we shouldn't go crazy with color variations.
See: Gist with colors pallete

15 default colors

color-palette-1

25 default colors

color-pallette-2

2. Color grid size

I've researched others color/background background features and there are a lot of differences from google docs 16px to even 32px, I'm not quite sure if 25px fits well in our case.

Anyway, still it will be possible to change this size via CSS Variable.

25px

color-palette-1

20px

20px-grid

3. Remove button position

There is question how often users will use this button - instead of using default color/background of text (black or white).

Just an idea: with CSS flex possibilities we can easily manipulate it via order property and introduce this option to the config.

Top

remove-button-top

Bottom

remove-button-bottom

4. Focused single color

I decided to make a border as outline instead of inner border, it's more visible.

single-color-focus

@msamsel
Copy link
Contributor

msamsel commented Mar 15, 2019

@dkonopka @mlewand
Above we have defined:

Column count should be figured out automatically based on config.

Should be allow customised amount of colors in panel or we should stick to 5? WDYT?

@msamsel
Copy link
Contributor

msamsel commented Mar 15, 2019

@dkonopka what with "last color" visualisation. Does the toolbar icon will get some sort of color indicator on it?

@msamsel
Copy link
Contributor

msamsel commented Mar 15, 2019

Also should there be any indicator that current selection has given color?
E.g. Imagine that in point 4, entire text is green, and current selection is in green text. Shouldn't be marked it somehow, that current selection is green?

@oleq
Copy link
Member Author

oleq commented Mar 18, 2019

@dkonopka Great research!

  1. I'd stick with the smaller palette. If an integration needs more, they'd probably define their own palette because it must match a specific set of colors (of the company, CI, etc.). We can leave a bigger palette as an example in our documentation.

  2. I wanted to say less is more but now I figured the "remove color" label could be much longer in some languages and it will stretch the layout. Maybe we can start with 20px but the layout must be flexible (CSS grid?) so if the label is longer, the grid adjusts to it, either by

    • increasing the number of columns
    • increasing the size of the color

    This is also closely related to:

    Column count should be figured out automatically based on config.

    can you research this, @dkonopka? Which option is possible?

  3. Top feels more natural to me but I'm open to any position.


what with "last color" visualisation. Does the toolbar icon will get some sort of color indicator on it?

AFAIR we decided that MVP has a static icon.

Also should there be any indicator that current selection has given color?
E.g. Imagine that in point 4, entire text is green, and current selection is in green text. Shouldn't be marked it somehow, that current selection is green?

Isn't the button pushed ("on") enough in the MVP?

@msamsel
Copy link
Contributor

msamsel commented Mar 18, 2019

what with "last color" visualisation. Does the toolbar icon will get some sort of color indicator on it?

AFAIR we decided that MVP has a static icon.

ok 👍

Also should there be any indicator that current selection has given color?
E.g. Imagine that in point 4, entire text is green, and current selection is in green text. Shouldn't be marked it somehow, that current selection is green?

Isn't the button pushed ("on") enough in the MVP?

Am I understand you correctly that you meant status of split button? I wonder how clear for the user will be to distinguish difference between no-color and black color of the text.

Another problematic case might be behaviour applying lastly used color.

  1. Let's say I have text marked on red.
  2. I make somewhere else changes and apply green color. (So now split button preserve green color for last apply it, without opening dropdown)
  3. Now I want to color this red text. As this element already has given color, status of the button should be pressed or not, as the color doesn't match with lastly used color?
  4. What if text is black and visually does not differ from text without color, but icon status is changing.

And another question should "remove color" be treated as "last color" and should be possible to apply directly from splitbutton?

@dkonopka
Copy link
Contributor

Column count should be figured out automatically based on config.

With CSS Grid we can easy control it via variable, take a look and try to uncomment some variables:

https://codepen.io/k0n0pka/pen/xBzPoe

@msamsel
Copy link
Contributor

msamsel commented Mar 18, 2019

Column count should be figured out automatically based on config.

With CSS Grid we can easy control it via variable, take a look and try to uncomment some variables:

https://codepen.io/k0n0pka/pen/xBzPoe

It's amazing sample :) I really like it and want to use it!
However now keeping config color data in 2-dimensional array seems to be to complex, just to get maximal amount of columns. And also not utilise situation when specific rows might have different amount of columns. I feel like we should have single dimension array and expose variable to set amount of columns?
@mlewand, @oleq, @dkonopka WDYT?

@oleq
Copy link
Member Author

oleq commented Mar 20, 2019

I see two cases here:

On one hand, we want a simple color configuration, right? In this case, the column number should be pre–defined IMO and as simple as possible.

On the other hand, some people may want to configure the colors so they align in a palette, for instance, the grayscale in a single row, or different saturations of the same color in the same column. To do that, people must be able to define both the colors and the number of columns.


I'm cool with the editor.config controlling the colors and CSS variable controlling the number of columns. This way simple integrations won't need lots of configuration and complex use–cases will be possible too. Does that make sense to you, guys?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests

6 participants