T/11: Implemented CSS variables as highlight color property #13
Conversation
dkonopka
changed the title
theme/highlight.css
Outdated
| */ | ||
|
|
||
| :root { | ||
| --ck-marker-yellow: #fdfd77; |
There was a problem hiding this comment.
I'd prefix it with --ck-highlight-(marker|pen)- to stick to the convention we use.
src/highlight.js
Outdated
| * type: 'marker' | ||
| * } | ||
| * | ||
| * @typedef {Object} module:highlight/highlight~HighlightOption | ||
| * @property {String} title The user-readable title of the option. | ||
| * @property {String} model The unique attribute value in the model. | ||
| * @property {String} color The color used for the highlighter. It should match the `class` CSS definition. | ||
| * @property {String} color The name of CSS variable used for the highlighter. It should match the `class` CSS definition. |
There was a problem hiding this comment.
I think this is wrong for 2 reasons:
- We must still allow developers to use colors in hex/rgba/hsl.
- Even if the above wasn't true, we have an entire
var()statement instead of the name of the variable.
We should make it clear that all hex/rgba/hsl/var() are possible.
src/highlight.js
Outdated
| * ] | ||
| * | ||
| * There are two types of highlighters available: | ||
| * - `'marker'` - rendered as a `<mark>` element, styled with the `background-color`, | ||
| * - `'pen'` - rendered as a `<mark>` element, styled with the font `color`. | ||
| * | ||
| * **Note**: A style sheet with CSS classes is required for the configuration to work properly. | ||
| * The highlight feature does not provide the actual styles by itself. | ||
| * The highlight feature does provide actual stylesheet with default colors. | ||
| * | ||
| * **Note**: It is recommended that the `color` value should correspond to the class in the content |
There was a problem hiding this comment.
We must explain here that we already do that thanks to var() but if you define colors with hex/rgba/hsl, you must take care of that by yourself.
src/highlight.js
Outdated
| * ] | ||
| * | ||
| * There are two types of highlighters available: | ||
| * - `'marker'` - rendered as a `<mark>` element, styled with the `background-color`, | ||
| * - `'pen'` - rendered as a `<mark>` element, styled with the font `color`. | ||
| * | ||
| * **Note**: A style sheet with CSS classes is required for the configuration to work properly. | ||
| * The highlight feature does not provide the actual styles by itself. | ||
| * The highlight feature does provide actual stylesheet with default colors. |
There was a problem hiding this comment.
s/does provide actual stylesheet/provides the style sheet/
docs/features/highlight.md
Outdated
| { model: 'blueMarker', class: 'marker-blue', title: 'Blue marker', color: '#72cdfd', type: 'marker' }, | ||
| { model: 'redPen', class: 'pen-red', title: 'Red pen', color: '#e91313', type: 'pen' }, | ||
| { model: 'greenPen', class: 'pen-green', title: 'Green pen', color: '#118800', type: 'pen' } | ||
| { model: 'yellowMarker', class: 'marker-yellow', title: 'Yellow Marker', color: 'var(--ck-marker-yellow)', type: 'marker' }, |
There was a problem hiding this comment.
There's not a single word in the documentation that the highlight feature comes with a style sheet, which defines a couple of markers and pens using CSS variables (a link to this page will be necessary too, it's a new thing around the web).
This must be clear, probably a new sub-section under Configuring the highlight options.
There was a problem hiding this comment.
You are right, I'll add example with configuring other than default colors 👍
|
Some errors in JSDoc |
|
Such link is volatile so it's not a good idea. There's no way to validate it in the future and tell if it actually works and see if it references the right page. The API docs must be self-explanatory. If they link outside, it usually indicates they are not good enough. Please, try rewording the docs to make them understandable and keep in mind that API docs are about the API and should not do the job of the guides. |
…semantics of the documentation.
Suggested merge commit message (convention)
Other: Changed the way of defining highlight marker and pen colors. Now by default, it is possible with CSS variables. Closes ckeditor/ckeditor5#2596.