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

The active font color is only selected in the document colors #2299

Closed
Reinmar opened this issue Jun 27, 2019 · 7 comments · Fixed by ckeditor/ckeditor5-font#53
Closed

The active font color is only selected in the document colors #2299

Reinmar opened this issue Jun 27, 2019 · 7 comments · Fixed by ckeditor/ckeditor5-font#53
Assignees
Labels
package:font status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jun 27, 2019

This seems wrong to me. Because you will never see the checkmark in the normal grid. It will always be in the document colors.

image

@msamsel
Copy link
Contributor

msamsel commented Jun 27, 2019

@Reinmar It was decided to have only one checkbox:
https://github.com/ckeditor/ckeditor5-font/issues/28#issuecomment-492597270
And you can see checkmark in normal grid if you have more colors than document color limit.
For example:
Screenshot 2019-06-27 at 09 16 24

@oleq
Copy link
Member

oleq commented Jun 28, 2019

IMO they should both have checkmarks (like in Sketch)

Screenshot 2019-06-28 at 11 47 18

@Reinmar
Copy link
Member Author

Reinmar commented Jun 28, 2019

Are there any arguments for not selecting both items?

@Reinmar
Copy link
Member Author

Reinmar commented Jun 28, 2019

cc @mlewand @dkonopka who were involved in the initial discussion.

@mlewand
Copy link
Contributor

mlewand commented Jul 11, 2019

It was actually my suggestion to implement it with a single checkmark. The reason is that we have a relatively heavy way to indicate selected color and having both of these looked heavy/messy.

@msamsel can you check out to a version before this change and provide a screen of a entire editor with a dropdown where both colors are selected? And next to it put a current screen (with only one selected) so we can compare it.

@msamsel
Copy link
Contributor

msamsel commented Jul 11, 2019

Screenshot 2019-07-11 at 14 52 40
Screenshot 2019-07-11 at 14 53 05

@oleq
Copy link
Member

oleq commented Jul 15, 2019

I'm for the later.

oleq referenced this issue in ckeditor/ckeditor5-ui Aug 9, 2019
Fix: `ColorGrid` should set the `#isOn` property value of new `ColorTiles`. See ckeditor/ckeditor5-font#51.
oleq referenced this issue in ckeditor/ckeditor5-font Aug 9, 2019
Other: The active color should be marked both in the document colors and all colors. Closes #51.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-font Oct 8, 2019
@mlewand mlewand added status:discussion 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 26 milestone Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:font status:discussion 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.

4 participants