fix: ensure circle elements in SVGs get currentColor fill #2687

adamstankiewicz · 2023-10-04T21:18:50Z

Description

Hi All, I'm seeing some people without heads in our icon library:

It looks our svgr config for the icons build process doesn't add fill="currentColor to elements in the SVGs, like it does for <path> elements. Implication: any icon with a circle(s) is likely currently missing the circle(s) when rendered.

Deploy Preview

https://deploy-preview-2687--paragon-openedx.netlify.app/foundations/icons

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

netlify · 2023-10-04T21:18:54Z

✅ Deploy Preview for paragon-openedx ready!

Name	Link
🔨 Latest commit	`6bd462e`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/6579700191e73300087521ab
😎 Deploy Preview	https://deploy-preview-2687--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

adamstankiewicz · 2023-10-04T21:34:34Z

icons/.svgrrc.js

@@ -56,7 +56,7 @@ module.exports = {
        [
          "@svgr/babel-plugin-add-jsx-attribute",
          {
-            "elements": ["path"],
+            "elements": ["path", "circle"],


This is the actual fix here :) All other files changed are the result of running npm run build in Paragon's icons folder.

codecov · 2023-10-04T21:41:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e00cb74) 92.83% compared to head (6bd462e) 92.83%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2687   +/-   ##
=======================================
  Coverage   92.83%   92.83%           
=======================================
  Files         235      235           
  Lines        4245     4245           
  Branches     1032     1032           
=======================================
  Hits         3941     3941           
  Misses        300      300           
  Partials        4        4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kalebjdavenport

I was nervous when I first opened the PR haha

viktorrusakov · 2023-10-05T07:06:08Z

icons/jsx/index.jsx

@@ -472,7 +472,6 @@ export { default as Check } from "./Check";
 export { default as CheckBoxIcon } from "./CheckBoxIcon";
 export { default as CheckBoxOutlineBlank } from "./CheckBoxOutlineBlank";
 export { default as CheckCircle } from "./CheckCircle";
-export { default as CheckCircleLightOutline } from "./CheckCircleLightOutline";


I assume this in not intended? (same comment for icons/es5/index.js file)

I think someone manually added jsx and js files for this icon but did not add the actual svg file to svg folder, so npm run build removes this import...

@viktorrusakov You are correct. Unfortunately, these lines will likely continue to be added until the appropriate SVG file is added...

Yeah, I'm just worried about making a breaking change here in this PR (Usage insights say that this icon is used in at least one MFE). Can you maybe manually remove these changes, so we can safely merge this? And then we create a separate issue to add the missing SVG file.

if we get #2869 merged we don't need to worry about this being a breaking change

@brian-smith-tcril Yes, you're right, I merged it so there shouldn't be any problems now.

I took the liberty of updating this PR myself by rebasing it on master as I know @adamstankiewicz is quite busy with enterprise work and might not get the chance to get to this PR soon. I validated that running npm run build does not remove any icons anymore and that the fix works (this is one of the fixed icons that Adam added to the PR description)

I'm going to merge it in as it is quite an important fix and PR has been open for a long time now.

brian-smith-tcril

openedx-semantic-release-bot · 2023-12-13T09:11:24Z

🎉 This PR is included in version 21.10.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

openedx-semantic-release-bot · 2023-12-21T07:03:49Z

🎉 This PR is included in version 22.0.0-alpha.23 🎉

The release is available on:

Your semantic-release bot 📦🚀

adamstankiewicz changed the title ~~fix: ensure color elements in SVGs get currentColor fill~~ fix: ensure circle elements in SVGs get currentColor fill Oct 4, 2023

adamstankiewicz added the bug Report of or fix for something that isn't working as intended label Oct 4, 2023

adamstankiewicz self-assigned this Oct 4, 2023

adamstankiewicz commented Oct 4, 2023

View reviewed changes

kalebjdavenport approved these changes Oct 5, 2023

View reviewed changes

viktorrusakov reviewed Oct 5, 2023

View reviewed changes

brian-smith-tcril approved these changes Oct 11, 2023

View reviewed changes

brian-smith-tcril mentioned this pull request Nov 27, 2023

fix: properly generate CheckCircleLightOutline icon from svg #2869

Merged

10 tasks

fix: ensure color elements in SVGs get currentColor fill

6bd462e

viktorrusakov force-pushed the ags/icons-circle-color branch from e79e901 to 6bd462e Compare December 13, 2023 08:49

viktorrusakov approved these changes Dec 13, 2023

View reviewed changes

viktorrusakov merged commit ef8a4cf into master Dec 13, 2023

viktorrusakov deleted the ags/icons-circle-color branch December 13, 2023 09:01

openedx-semantic-release-bot added the released label Dec 13, 2023

openedx-semantic-release-bot added the released on @alpha label Dec 21, 2023

khudym pushed a commit to khudym/paragon that referenced this pull request Jan 2, 2024

fix: ensure color elements in SVGs get currentColor fill (openedx#2687)

1a0cde9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure circle elements in SVGs get currentColor fill #2687

fix: ensure circle elements in SVGs get currentColor fill #2687

adamstankiewicz commented Oct 4, 2023 •

edited

Loading

netlify bot commented Oct 4, 2023 •

edited

Loading

adamstankiewicz Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

kalebjdavenport left a comment

viktorrusakov Oct 5, 2023 •

edited

Loading

adamstankiewicz Oct 13, 2023

viktorrusakov Oct 18, 2023 •

edited

Loading

brian-smith-tcril Nov 27, 2023

viktorrusakov Dec 13, 2023

brian-smith-tcril left a comment

openedx-semantic-release-bot commented Dec 13, 2023

openedx-semantic-release-bot commented Dec 21, 2023

fix: ensure circle elements in SVGs get currentColor fill #2687

fix: ensure circle elements in SVGs get currentColor fill #2687

Conversation

adamstankiewicz commented Oct 4, 2023 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

netlify bot commented Oct 4, 2023 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

adamstankiewicz Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

kalebjdavenport left a comment

Choose a reason for hiding this comment

viktorrusakov Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz Oct 13, 2023

Choose a reason for hiding this comment

viktorrusakov Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

brian-smith-tcril Nov 27, 2023

Choose a reason for hiding this comment

viktorrusakov Dec 13, 2023

Choose a reason for hiding this comment

brian-smith-tcril left a comment

Choose a reason for hiding this comment

openedx-semantic-release-bot commented Dec 13, 2023

openedx-semantic-release-bot commented Dec 21, 2023

adamstankiewicz commented Oct 4, 2023 •

edited

Loading

netlify bot commented Oct 4, 2023 •

edited

Loading

adamstankiewicz Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

viktorrusakov Oct 5, 2023 •

edited

Loading

viktorrusakov Oct 18, 2023 •

edited

Loading