fix(react-twig): refactor light social media icon color #1179
Conversation
🦋 Changeset detectedLatest commit: 7f243ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for ilo-ui-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ilo-ui-twig ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ilo-ui-twig-develop ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| $icon, | ||
| map-get($color, "socialmedia", $theme, "icon", "color") | ||
| ); | ||
| @include dataurlicon($icon, rgba(30, 45, 190, 1)); |
There was a problem hiding this comment.
Hey team since we are moving away from map-get and using css variables, there a small problem in this one. Css variables cant be used in mixins.
For the time being I used rgba(30, 45, 190, 1) as it doesn't support hex values as well.
How should we go with this one @justintemps @bashlk @GGKapanadze
|
@justintemps I tried passing the variable as a string but it doesn't seem to work. So for the time being I created a scss variable. |
Shashika6
force-pushed
the
fix/react-twig/social-media
branch
from
216ab88 to
9ba916e
Compare
| @@ -7,9 +7,10 @@ | |||
| $c: &; | |||
| $default-theme: "light"; | |||
| $icon-size: $spacing-spacing-4; | |||
| $ilo-color-blue: "rgba(30, 45, 190, 1)"; | |||
There was a problem hiding this comment.
I think what the guys meant is to assign the CSS variable to the SCSS variable so that it can be passed into the mixin.
There was a problem hiding this comment.
You mean $ilo-color-blue: var(--ilo-color-blue); right ? I tried this but it didnt seem to work.
|
@Shashika6 also I think you forgot to implement the dark theme version |
|
@bashlk, @Shashika6 and @GGKapanadze, the problem here isn't that css vars can't be used in mixins, the problem is that they don't work inside a data url, which is what the icon function and mixins are doing. They give us something like this: background-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 24 21'%3e%3cpath fill='var(--ilo-color-blue)' d='M18.901 0.153564H22.581L14.541 9.34356L24 21.8466H16.594L10.794 14.2626L4.156 21.8466H0.474L9.074 12.0166L0 0.154564H7.594L12.837 7.08657L18.901 0.153564ZM17.61 19.6446H19.649L6.486 2.24056H4.298L17.61 19.6446Z'/%3e%3c/svg%3e");If you see, the variable is getting passed appropriately, but because the SVG isn't in the DOM per se, it doesn't work. I think this reveals a small but important flaw in our new approach for handling tokens, in that css variables can't be used in some contexts. I'm not sure what the best way to handle this is. |
|
Yep, variables can't be directly used with fill, but if we still want to do background images for icons I can reimplement this mixin with 'mask-image' |
|
Marking this as a draft till #1189 is completed. |
Fixes #1012
Notes