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 Marker Needs Option For Text Color #1030

Closed
duracell80 opened this issue May 21, 2018 · 4 comments
Closed

Highlight Marker Needs Option For Text Color #1030

duracell80 opened this issue May 21, 2018 · 4 comments
Labels
domain:accessibility This issue reports an accessibility problem. resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:discussion

Comments

@duracell80
Copy link

duracell80 commented May 21, 2018

There seems to be a missing variable for the text color of the highlighter. Only the background color seems to be configurable, from looking at the documentation and examples all of the text is black. When the background is dark the text can't be black for accessibility reasons ( contrast ).

Suggest as an example:
--ck-highlight-marker-green-text: white

https://github.com/ckeditor/ckeditor5-highlight/blob/master/theme/highlight.css

@Reinmar
Copy link
Member

Reinmar commented May 22, 2018

cc @dkonopka @oleq

Ideally, we should avoid having too many variables due to the code size and just because there's never enough of them. Here, we'd need to add 6 more variables. Therefore, I'd rather like to avoid that. We can mention in the documentation that if you want to override more than the bg color you should do that by overriding these entire selectors. WDYT?

@oleq
Copy link
Member

oleq commented May 22, 2018

I'd rather keep things simple if that's possible.

I'm pretty sure no integration will use more than 10 of markers/pens so it's not a huge chore to maintain them. What we have in https://github.com/ckeditor/ckeditor5-highlight/blob/master/theme/highlight.css is just a default set of markers/pens and many integrations will develop their own sets anyway.

@Reinmar
Copy link
Member

Reinmar commented Oct 29, 2019

Let's merge this into #5677

@Reinmar Reinmar added the resolution:duplicate This issue is a duplicate of another issue and was merged into it. label Oct 29, 2019
@oleq
Copy link
Member

oleq commented Oct 29, 2019

I'm still against creating variables for such purposes and given that there was no more feedback since the last year, I decided to mark it as invalid.

@oleq oleq closed this as completed Oct 29, 2019
@oleq oleq added resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). and removed resolution:duplicate This issue is a duplicate of another issue and was merged into it. labels Oct 29, 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. resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:discussion
Projects
None yet
Development

No branches or pull requests

3 participants