Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Color Module - First Draft #147
base: main
Are you sure you want to change the base?
Color Module - First Draft #147
Changes from 29 commits
cb13637
e674eeb
fd0e930
0e2cc98
055697c
bc0f325
0f02b4c
ec0079e
b688a40
8ce0869
e4283bc
b938bb9
2f4349c
f4728eb
d25486c
f76ef65
a618f5f
3bb8998
70c7212
d707312
c5eeb7e
f34cd40
1b18504
15d896a
5b65608
e47765b
e7a9c30
62b7771
c61fab2
c58ca34
1226b98
1195a2f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The wording is just a suggestion and can be entirely rewritten (please change / improve / rewrite everything!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colorSpace
andchannels
together, since they are validated together:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: make a markdown table that restates the information from CSS Color Module Level 4 that contains:
colorSpace
values acceptedcolorSpace
value, the min/max values allowed per channel. This is important because it won’t always be[0, 1]
(e.g. the “H” in HSL is[0, 360]
).Note: the column names may change to whatever is most readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: should we explain why it’s an array, and why the order matters (following CSS Color 4 spec)? Or is that too much information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alpha
section to describe this property briefly. Also maybe recommend CSS Color 4’s alpha interpolation section?All of the phrasing is suggested and may be completely rewritten. Just communicates some things to think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To align with the current spec draft and, as far as I know, how hex color values are interpreted in CSS and most design tools, should we say here that the sRGB color space is always implied for these fallback hex values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Hex values have started to be used for Display P3 in design tools, but the intent is limited to sRGB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: remove this entire section, including all the colorspaces? (this can be found in the
colorSpace
table)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest splitting this out into a separate PR for now.
As it stands, this has got quite a few deviations from how the gradient type is currently defined in the format:
$
prefixes for the value's inner properties and, as per the comments from @drwpow and myself on the updated color type, I think we should stick with that. (or, if we didn't, we'd need to change all the other composite types to be consistent)style
property, though several folks have suggested something along those lines in Gradient type feedback #101. I think that's probably a worthwhile addition, but there are other interesting proprosals in that thread too, which I think we ought to consider holistically.alpha
property on the stops and, given that the color values can have an alpha channel, it feels kinda redundant. Besides, what would be the expected behaviour if both have been specified but have conflicting values?I presume the intent of this section is really just to show that the new, expanded color value syntax defined in the previous chapter can be used anywhere else that color values are permitted.
The
value
of a gradient stop is one case of that, but there are others too:color
sub-value of a shadowcolor
sub-value of a borderSo, perhaps this section could be reworked into something that just states that the new object-style color values can be used throughout the spec and include an example of each of those things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
composite-types.md
—updating all those examples to reference valid colors