Remove remaining color sass function uses #4655
Conversation
| // This is the only place this color is used. | ||
| // stylelint-disable-next-line color-no-hex | ||
| $item-selected-background: rgba(color('indigo'), 0.12); | ||
| $item-selected-background: rgba(var(--p-background-selected), 0.12); |
There was a problem hiding this comment.
this isn't actually used anywhere but assuming this should be removed in a followup PR
There was a problem hiding this comment.
Only files referenced by src/styles/_public-api.scss are part of the public API. I've not checked but I suspect this file is only used by the styles for specific components (e.g. Navigation/Navigation.scss), and is never referenced by _public-api.scss. If that is the case then we are free to delete this in the main branch without any backwards compatability concerns and you are free to nuke it now.
There was a problem hiding this comment.
confirmed that this is not referenced in the _public-api.scss file (and isn't even used by Navigation.scss or elsewhere. Removing this line entirely.
size-limit report
|
src/styles/shared/_buttons.scss
Outdated
|
|
||
| @mixin button-outline-disabled($outline-color) { | ||
| background: transparent; | ||
| // border-color: rgba($outline-color, 0.25); |
There was a problem hiding this comment.
We should probably get this line too!
src/components/Button/Button.scss
Outdated
| // Because the outline border color has a 40% opacity, the left border appears darker than the rest of the borders because they are layered over one another. Reducing the opacity to zero for the connected disclosure when not focused gives us the expected border color. | ||
| &.outline:not(:focus) { | ||
| border-left-color: rgba(color('ink', 'lighter'), 0); | ||
| border-left-color: rgba(var(--p-border-disabled), 0); |
There was a problem hiding this comment.
I believe rgba is a sass function, I don't think it will have the desired effect when you pass in a css variable, as the color won't be resolved till runtime
There was a problem hiding this comment.
I didn't know this! Thanks @BPScott
@alex-page what's the best way forward on this? hard code the color? leave for now and create a new issue? is rgba() going to be removed eventually as well?
There was a problem hiding this comment.
Lets hard code the color to unblock this PR. It will be a failure on polaris-coverage so we won't miss it.
| // This is the only place this color is used. | ||
| // stylelint-disable-next-line color-no-hex | ||
| $item-selected-background: rgba(color('indigo'), 0.12); | ||
| $item-selected-background: rgba(var(--p-background-selected), 0.12); |
There was a problem hiding this comment.
Only files referenced by src/styles/_public-api.scss are part of the public API. I've not checked but I suspect this file is only used by the styles for specific components (e.g. Navigation/Navigation.scss), and is never referenced by _public-api.scss. If that is the case then we are free to delete this in the main branch without any backwards compatability concerns and you are free to nuke it now.
yurm04
force-pushed
the
follow-up-color-filter
branch
from
f53f6e2 to
1cd6488
Compare
BPScott
left a comment
BPScott
left a comment
There was a problem hiding this comment.
Seems we never use those arguments on button-outline, and button-outline-disabled. Perhaps we should just stop passing them in entirely?
We can't remove the parameter from the function on main as that would be a breaking change, but we could stop passing any value in when we call it
src/components/Button/Button.scss
Outdated
|
|
||
| &.disabled { | ||
| @include button-outline-disabled(color('ink', 'lighter')); | ||
| @include button-outline-disabled(var(--p-border-disabled)); |
There was a problem hiding this comment.
The $outline-color parameter doesn't seem to be used anymore. https://github.com/Shopify/polaris-react/blob/main/src/styles/shared/_buttons.scss#L198-L205
Perhaps we can just remove passing in the argument entirely: @include button-outline-disabled()
There was a problem hiding this comment.
Alternatively this seems to be the only place this mixin is used - perhaps we just inline it here as all it does is background: transparent; box-shadow: none;
| } | ||
|
|
||
| .outline { | ||
| @include button-outline(color('ink', 'lighter')); |
There was a problem hiding this comment.
The $outline-color parameter doesn't seem to be used anymore. https://github.com/Shopify/polaris-react/blob/main/src/styles/shared/_buttons.scss#L118
Perhaps we can just remove passing in the argument entirely: @include button-outline()
|
|
||
| .destructive.outline { | ||
| @include button-outline(color('red')); | ||
| @include button-outline(var(--p-border-critical)); |
yurm04
force-pushed
the
follow-up-color-filter
branch
from
71dfa0e to
9c92aec
Compare
Co-authored-by: Alex Page <hi@alexpage.dev>
3 participants
WHY are these changes introduced?
Follow up to #4636, there were a few
color()functions that got left behind. Cleaning up the rest.How to 🎩
Run storybook and check Chromatic for visual regressions