Skip to content

Conversation

@yurm04
Copy link
Contributor

@yurm04 yurm04 commented Nov 16, 2021

WHY are these changes introduced?

Related to #4528
Fixes filter() part of #4592

WHAT is this pull request doing?

Removing all instances of filter() Sass function because the color system used by this function has been deprecated. Swapping out any references to filter() with the color system custom css properties

How to 🎩

Most changes are in the Navigation component. Run Storybook and/or visit the Chromatic link to this branch to ensure there are no visual regressions.

@yurm04 yurm04 requested review from alex-page and lgriffee November 16, 2021 18:25
&:hover,
&:focus,
&:active {
@include recolor-icon(
Copy link
Member

Choose a reason for hiding this comment

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

Is this function used a bunch? I would assume it's a filter related function. If it is we could move it to this file.

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.

I think we should remove the filter() function and point this to v8.0.0-major branch. Looking great so far! We will have to add documentation for migration and a breaking change to the changelog.

@yurm04 yurm04 changed the base branch from main to v8.0.0-major November 16, 2021 21:50
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.

Looks great for the first cut of removing the filter function references!

The files changed need a rebase/merge or something 👍

@yurm04 yurm04 changed the base branch from v8.0.0-major to main November 16, 2021 22:02
@github-actions
Copy link
Contributor

size-limit report

Path Size
cjs 165.95 KB (0%)
esm 96.52 KB (0%)
esnext 143.09 KB (-0.02% 🔽)
css 34.28 KB (-0.04% 🔽)

@yurm04 yurm04 merged commit 2ab6b9f into main Nov 16, 2021
@yurm04 yurm04 deleted the remove-filter-functions branch November 16, 2021 22:06
@include recolor-icon(
$fill-color: var(--p-icon-on-interactive),
$filter-color: filter('white')
$filter-color: var(--p-icon-on-interactive)
Copy link
Member

Choose a reason for hiding this comment

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

These filter-color values passed to the 3rd argument of recolor-icon are sent as the filter property. They're not actual color values, they're filters like brightness(0) saturate(100%) invert(100%).

I think changing these to colors is going to break the behaviour, which is to allow for the recoloring of icons that are passed as svg datauris and rendered with an img tag

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.

3 participants