Skip to content

Conversation

@yurm04
Copy link
Contributor

@yurm04 yurm04 commented Nov 12, 2021

WHY are these changes introduced?

Fixes #4592

The color() Sass function should not be used as it uses a deprecated color system. Replacing all color() invocations in favor of the up to date color system that uses custom css properties

WHAT is this pull request doing?

Swapping color() for equivalent css custom properties.

Affected components

  • AppProvider
  • MappedOptions
  • Badge
  • Navigation
  • DualThumb

How to 🎩

  1. Run the Storybook
  2. Go through each of the affected components listed above. There should be no major visual differences

@yurm04 yurm04 requested review from alex-page and lgriffee November 12, 2021 19:03
@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2021

size-limit report

Path Size
cjs 165.95 KB (0%)
esm 96.52 KB (0%)
esnext 143.11 KB (-0.03% 🔽)
css 34.29 KB (-0.12% 🔽)

@yurm04 yurm04 requested review from BPScott and alex-page November 15, 2021 22:00
@yurm04 yurm04 force-pushed the remove-color-sass-function branch from 599d5f7 to 7a93f3d Compare November 15, 2021 22:39
@yurm04 yurm04 requested a review from alex-page November 16, 2021 17:59
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing left is to remove the function. We could point this to the v8.0.0-major branch.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the color-icon mixin is a breaking change to our public sass API - consumers used to be able to use that mixin and now it's gone. Up to y'all decide if that's acceptable in a minor release.

Personally I'd err towards trying to make sure these removal PRs occur in the main branch rather than a v8 branch (as then there will be fewer merge headaches in the future), but try to maintain backwards compatibility in minor versions - And thus leave the color-icon mixin hanging around, referencing the color function. Perhaps add a @deprecated docblock to color/color-icon, and have a follow-up PR that removes color/color-icon targeting a v8 branch.


Not blocking for this PR but an errant thought bouncing around my brain:

It'd be neat to remove the ability to use color() internally to make sure usage doesn't creep back in (unlikely with color, but possible for other functions that are due to become tokens). As alluded to earlier _public-api.scss is our public api, and removing items from that file or the files referenced therein is a breaking change. However we don't use that file internally. Internal sass API usage is done by importing _common.scss.

By removing the @import './foundation/colors'; from _common.scss then internal files would not have access to the functions used within it.

I think a bit of extra work is needed to get to that point - either removing the usage of the other functions defined in that file, or splitting the files up so some functions are only exposed via _public-api.scss. I'm not sure quite sure how that'd look off the top of my head, or if it'd be worth doing as you go or waiting till most of these functions have been converted to tokens.

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets break a part removing functions for the v8 branch. Thanks @BPScott and @yurm04

@yurm04 yurm04 merged commit 7d44e63 into main Nov 16, 2021
@yurm04 yurm04 deleted the remove-color-sass-function branch November 16, 2021 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove legacy functions color() and filter()

3 participants