Improve CSS vars #528
Improve CSS vars #528
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ Coverage Diff @@
## main #528 +/- ##
==========================================
- Coverage 75.53% 75.49% -0.04%
==========================================
Files 41 41
Lines 7017 7008 -9
==========================================
- Hits 5300 5291 -9
Misses 1717 1717
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…other components.
Co-authored-by: Gary Katsevman <git@gkatsev.com>
| | `--media-button-icon-transition` | `transform` | none | apply a [transition](https://developer.mozilla.org/en-US/docs/Web/CSS/transition) to button icons | Only applies to `<img>` and `<svg>` tags | | ||
| | Name | CSS Property | Default Value | Description | Notes | | ||
| | ---------------------------------- | ------------- | -------------------------------------------------- | ------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `--media-control-background` | `background` | `var(--media-secondary-color, rgb(20 20 30 / .7))` | background color of the component | Applies to other components as well ([See notes below \*\*](#notes)) | |
There was a problem hiding this comment.
should --media-primary-color and --media-secondary-color get their own section at the top?
There was a problem hiding this comment.
Looks like the new font css vars aren't documented either. Thinking about this, might be fine to skip for now since we have an upcoming effort to update documentation for this.
| <style> | ||
| :host { | ||
| font: var(--media-font, |
There was a problem hiding this comment.
At first, I was worried that moving into the shorthand property it'll cause issues if someone were to set a specific property or the font property on the element, but after trying it out, that seems unfounded and working as expected.
| <style> | ||
| :host { | ||
| font: var(--media-font, |
There was a problem hiding this comment.
I think I would still slightly prefer individual properties over the font shorthand. It's more explicit and would help with searchability, since you could find it via grepping for say line-height.
There was a problem hiding this comment.
cool will update. I don't feel so strongly about it
There was a problem hiding this comment.
oh actually then we can't use --media-font as a CSS var for setting all at once which includes some more things like
font-stretch
font-style
font-variant
I think that extra function and it taking less CSS makes it a better choice. what do you think?
There was a problem hiding this comment.
Theoretically, we could do some fancy var stuff to unset the other properties if --media-font is set, but probably too much work. Is --media-font needed?
While, I do think the individual properties are a bit better, I'm OK with keeping it as the shorthand.
There was a problem hiding this comment.
Is --media-font needed?
it would give devs the full flexibility of all font css props if needed with small addition in code size.
seems like a good devex
| <style> | ||
| :host ul { | ||
| font: var(--media-font, |
There was a problem hiding this comment.
praise: As long as we don't run into any hurdles/friction, I think I'm digging this solution for shorthand vs. specific css props. 😎
|
the buttons are def covered via |
@luwes whoops just missed that in the quick review. 🚀 |
Closes #473
Fixes #453
--media-primary-color--media-secondary-color--media-tertiary-color--media-text-color--media-font,--media-font-weight,--media-font-size,--media-font-family