Add support for multiple box shadows on BoxShadow Modifier API #536
Conversation
There was a problem hiding this comment.
Thanks for this PR!
Phew, as you may have seen, 0.18.0 is finally out. Took quite a bit longer than I thought, so I wanted to say sorry for the delay.
Overall, this looks great, and I appreciate the time you spent on the header docs. I think we can get this into 0.18.1 fairly soon. There are some fixes that will have to happen first, but they should be fairly trivial, and hopefully helpful in understanding how nuanced this stuff can get sometimes.
Since this is our first time working together, just wanted to say feel free to ask questions or push back with your own suggestions anytime, happy to keep a dialog open.
frontend/compose-html-ext/src/jsMain/kotlin/com/varabyte/kobweb/compose/css/Box.kt
Show resolved
Hide resolved
frontend/compose-html-ext/src/jsMain/kotlin/com/varabyte/kobweb/compose/css/Box.kt
Outdated
Show resolved
Hide resolved
frontend/compose-html-ext/src/jsMain/kotlin/com/varabyte/kobweb/compose/css/Box.kt
Outdated
Show resolved
Hide resolved
frontend/compose-html-ext/src/jsMain/kotlin/com/varabyte/kobweb/compose/css/Box.kt
Show resolved
Hide resolved
frontend/compose-html-ext/src/jsMain/kotlin/com/varabyte/kobweb/compose/css/Box.kt
Outdated
Show resolved
Hide resolved
playground/site/src/jsMain/kotlin/playground/pages/Modifiers.kt
Outdated
Show resolved
Hide resolved
rafaeltonholo
force-pushed
the
feature/modifier-box-shadow-with-multiple
branch
from
cc1de93
to
555dde0
Compare
rafaeltonholo
force-pushed
the
feature/modifier-box-shadow-with-multiple
branch
from
555dde0
to
a9b76b0
Compare
rafaeltonholo
force-pushed
the
feature/modifier-box-shadow-with-multiple
branch
from
a9b76b0
to
007ef3f
Compare
There was a problem hiding this comment.
Looks great, and thanks for your patience on the back and forth.
I can tell by your attention to detail that wherever you work or whatever you work on is lucky to have you.
You may be interested to hear that the discussions we had here about the "CSS-" prefix has actually caused us to reflect on the pattern more broady in our codebase, and it seems likely that the way I designed CSSTransition and CSSAnimation is wrong.
Note
CSSAnimation comes from Compose HTML but they screwed up their implementation, which is originally what caused me to introduce a fixed version of it into Kobweb, but now we're thinking Animation.of is more Kobweb idiomatic.
| * } | ||
| * ``` | ||
| */ | ||
| fun Modifier.boxShadow(vararg shadows: BoxShadow.Shadow): Modifier = styleModifier { |
There was a problem hiding this comment.
Not 100% sure what I feel about the name BoxShadow.Shadow but I certainly won't block the PR over it! We can easily tweak this name later if we come up with something else.
There was a problem hiding this comment.
I've named it Shadow because that's how W3C refers to that property value. However, MDN uses spread-shadow instead, which could also be an alternative. Since W3C defines the web standard, I prefer sticking with their name. But I understand that it looks quite ugly as BoxShadow.Shadow.
Now that I think about it, maybe I should name it BoxShadow.Property instead, which is similar to what you're already doing with the other properties in the project. If you'd like, I can create a small PR to address that, or we can leave it as is and address it later.
I'm glad that I could help with the project! I really like its idea and I hope I can contribute more in the future :) |
Currently, the BoxShadow Modifier API only supports a single shadow. This PR enables the API to receive multiple shadows or
none, in case the user wants to remove a shadow in a variant style.