Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: convertColors plugin ignore currentColor in 'mask' elements #2032

Merged
merged 9 commits into from
Jun 14, 2024

Conversation

salvan13
Copy link
Contributor

@salvan13 salvan13 commented Jun 11, 2024

convertColors plugin ignore currentColor in mask elements since it makes them not working properly

fixes #2031

@salvan13 salvan13 changed the title added ignoreElement to convertColors plugin feat: ignoreElement parameter for convertColors plugin Jun 11, 2024
@salvan13 salvan13 changed the title feat: ignoreElement parameter for convertColors plugin feat: ignoreElements parameter for convertColors plugin Jun 11, 2024
@SethFalco
Copy link
Member

SethFalco commented Jun 13, 2024

Hey hey! Thanks for submitting a pull request!

Rather than adding the param, imo the plugin should just internally ignore masks for the convertColor option.

This was a use-case that was overlooked when the option was added, but I don't see a use-case to convert a mask to currentColor at all. (If that comes up in future, we can amend the convertColor option or introduce a new one to include masks.)

An issue with this solution is that it calls visitSkip, which stops all optimization. This would also stop it from applying any of the other options too, like converting white to #fff, which is valid for masks and shaves 1 byte.

A better way to approach it would be to declare a counter at the top that increments when an open mask tag is hit, and decrements when a close mask tag is hit, then only apply convertColor when that counter is 0. (We're not inside a mask.)

How does that sound to you? If you have any feedback or want guidance, feel free to ask!

@salvan13
Copy link
Contributor Author

Hello! Thanks for your reply, it sounds good to me!

I also think that there is no use case for mask to convert colors to currentColor, I'll try to implement it as you suggested without a config and with a counter to not apply currentColor to mask, I'll update the PR asap!

@salvan13 salvan13 changed the title feat: ignoreElements parameter for convertColors plugin feat: convertColors plugin ignore currentColor in 'mask' elements Jun 14, 2024
@salvan13
Copy link
Contributor Author

Hello @SethFalco I updated the PR, now it seems working great 😃
the mask colors are optimized but not changed to currentColor.
Thanks for your review!

Copy link
Member

@SethFalco SethFalco left a comment

Choose a reason for hiding this comment

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

Looks good to me! I've raised a minor issues with the types/defensive checks, but the logic is sound.

plugins/convertColors.js Outdated Show resolved Hide resolved
plugins/convertColors.js Outdated Show resolved Hide resolved
@SethFalco SethFalco merged commit e73d13a into svg:main Jun 14, 2024
12 checks passed
@SethFalco
Copy link
Member

Awesome, thank you very much for the contribution!

This will be releases in v4.0.0-rc.1, and eventually v4.0.0.

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.

Mask colors are changed when using convertColors and currentColor: true
2 participants