-
Notifications
You must be signed in to change notification settings - Fork 661
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
[css-images-4][css-backgrounds-4] Added stripes() function #7029
Conversation
Hi Sebastian, Thank you for working on this. |
Thank you for reviewing the PR! There are obviously some open questions, so let's continue the discussion in the issue before we continue with this PR. Sebastian |
c1ce848
to
119070c
Compare
@LeaVerou @tabatkins I went ahead and interpreted your statements in the issue to make the relevant changes. That means, I added I also added it to I am awaiting your feedback on these changes. Sebastian |
8be979c
to
8046368
Compare
As noted by @LeaVerou, I've now put Open questions:
Sebastian |
I don't understand either of the two questions, could you please elaborate? |
I used the double pipe in the syntaxes, i.e. That allows to specify something like We might use juxtaposing instead, i.e.
As noted in a previous comment, I reused Though, as it is now used in two completely different places, should it's description be generalized and be moved somewhere else? And if so, should that be done in this PR already? Sebastian |
Oh no, you need to use a single pipe:
Yeah, it should go to css-values. @fantasai @tabatkins ? |
Yeah we can move |
Great, should we call it something else in that case? |
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.
Overall looks pretty good. Posted some specific things to fix.
Also, the line-breaking convention for our specs is https://rhodesmill.org/brandon/2012/one-sentence-per-line/ (This is why the lines look so ragged, there's a method to the madness.)
(You might also find https://lists.w3.org/Archives/Public/spec-prod/2018OctDec/0011.html a useful read as background context, though nothing else there is particularly relevant to this PR.)
css-backgrounds-4/Overview.bs
Outdated
|
||
The stripes are painted above a given <<color>>. This allows the <<color>> | ||
value to serve as a fallback, so any transparent stripes let the color | ||
shine through. | ||
|
||
When interpolating between borders with the same number of colors, | ||
interpolation is performed individually per color band as <a href="https://www.w3.org/TR/css3-transitions/#animtype-color">color</a>. |
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.
Interpolation should probably be defined in the definition of the stripes()
function, rather than here. And it should match what we do for gradients.
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've moved it to the "Interpolation" section in CSS Images 4 and tried to align its definition with the one for gradients. @fantasai As those changes are bigger, please have a detailed look at it and let me know whether something needs to be changed!
Sebastian
I've done what turned out to be a decent bit of rearranging and rewording on the Images part. Largely editorial; the only normative changes are to handle sums of flexes < 1 the same way flexbox does, and define the painting area when used as a 2d image. |
I deliberately chose to allow both, so
That means, authors have to add the slashes when using
Sebastian |
As even Tab obviously mixed it up being part of Flexbox and not Grid, I'd say yes. 😉 Though I don't have a strong opinion on that. And I agree with him that this should be done in a separate PR. Sebastian |
Thanks for the links! I'll correct the line-breaking. Sebastian |
I missed that when answering the previous comments. @tabatkins Thank you for the changes! With those you obviously already covered a few things noted by @fantasai. I'll incorporate the rest of the changes and we can discuss the points that are still open afterwards. Sebastian |
What kind of fallback? If |
I don't know what the "color to paint the stripes over" is for, tbh. |
No idea, but I assume that's what @SebastianZ intended for the fallback, a color to paint underneath any semi-transparent stripes. FWIW I don't think it's that necessary and complicates the syntax, so I'd be fine removing it. |
Yes, the meaning of this "fallback color" was to let the color is drawn below any (semi-)transparent stripes. So, basically the same interaction as Though I'm happy to change the syntaxes to If we decide to use the either-or syntax instead, we should also clarify whether it's ok to stick with "transparent black" as fill color. See also @fantasai's related comment. Sebastian |
As the comment list on this PR got quite long, let me summarize the pending points:
@fantasai @tabatkins Could you please give this another review so the changes can be merged? Sebastian |
Animation type: see prose | ||
</pre> | ||
|
||
<pre class="propdef shorthand"> | ||
Name: border-color | ||
Value: <<color>>#{1,4} | ||
Value: [ <<color>> | <<1d-image>> ]{1,4} |
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.
@tabatkins @fantasai What do you think about my suggestion to distinguish the values for the different borders by adding slashes between them like in grid-area
?
This makes the syntax more future proof, in my opinion.
Sebastian
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.
For reference, here's the suggested syntax again:
<<color>>{1,4} | [ <<color>>? <<1d-image>>? ]! [ / [ <<color>>? <<1d-image>>? ]! ]{0,3}
And here's an example using that syntax:
border-color: stripes(white, silver) / stripes(white, silver) / stripes(black, gray) / blue;
css-images-4/Overview.bs
Outdated
The function represents a 1-dimensional image generated by a list of colors | ||
and an optional thickness for each of them. | ||
|
||
A <<percentage>> or <<flex>> value refers to the total length of the image | ||
in the used context. | ||
The <<percentage>> must be between ''0%'' and ''100%'' inclusive; | ||
any other value is invalid. | ||
|
||
If the thickness of a stripe is omitted, it is interpreted as ''1fr''. |
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 don't think this is still needed but please let me know if it is!
This was resolved on in w3c#2532 (comment).
fa2ec8b
to
eb59c87
Compare
If used generically as an <<image>>, | ||
the |painting area| is the height of the [=concrete image size=]. |
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.
What does "If used generically as an <image>
" mean, if <1d-image>
is no longer an <image>
?
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.
That was just a left-over. I've removed the whole paragraph now as it didn't add any additional information anymore.
Thanks for the hint!
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.
You removed another used generically as an <<image>>
, but not this one.
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 went ahead and merged the changes now as I believe all feedback was addressed and I waited for half a year for review. I'll create separate issues for my suggestion to change the syntax of If there is still anything left, please let me know and I'll change it. Sebastian |
If the thickness is omitted, | ||
it defaults to ''1fr''. | ||
|
||
In each entry, a <<percentage>> is relative to the |painting area| |
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 think this is very confusing, the "painting area" is never defined but since it's an area it seems to be 2D. How to resolve a 1D <length-percentage>
relatively to a 2D area?
Should this refer to the "length of the paint line" instead?
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.
If used generically as an <<image>>, | ||
the |painting area| is the height of the [=concrete image size=]. |
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.
You removed another used generically as an <<image>>
, but not this one.
|
||
2. Neither image uses a combination of <<length>>, <<percentage>>, and <<flex>> stripes. | ||
|
||
If the two image satisfy both constraints, |
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.
If the two images
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.
first match each stripe in the start image | ||
to the corresponding stripe at the same index in the end image. | ||
Then, for each pair of stripes, | ||
interpolate the <a href="https://www.w3.org/TR/css3-transitions/#animtype-length">thickness</a> |
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.
How to interpolate a <length-percentage>
with a <flex>
?
Rather than checking that each image doesn't mix different types, should the requirement be that each pair of matched stripes have the same type?
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.
This change is meant to convert the resolution of #2532 into spec. text.
It adds the
stripes()
function as a generally usable function as discussed in #2532 to CSS Images 4.Note that I changed the syntax slightly to incorporate the
fr
unit mentioned in @LeaVerou's description. I did that by adding the<flex>
value defined in CSS Grid 1, though the definition there currently focuses on the usage within grid layout. So its description either might need to be changed/generalized or the unit needs to be defined again within CSS Images 4. I'd prefer to generalize it (and maybe even move it to CSS Values 4).The
stripes()
function is then used as a replacement for the colors list defined forborder-*-color
in CSS Backgrounds 4.Reading through the discussions, a few things were still unclear to me or were discussed but not resolved on.
stripes()
function is used in is smaller or bigger? I've defined that the rest is transparent when the stripes thickness is smaller and that the stripes are clipped when they are bigger than the image, as that seems to be what people were in favor of in the discussion.border-color
) interact withborder-style
? Clipping was mentioned in the discussion, also treatinginset
,outset
,groove
, andridge
as solid.Also note that this change is based on #7014, because without those changes Bikeshed won't compile the spec.
Sebastian