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

refactor: import icons as svg files instead of using react components #6050

Closed
wants to merge 2 commits into from

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Dec 4, 2021

This pr changes how svg files are stored in project and removes all css from them, this is first step to refactor how we import icons.

as we are using svgr to convert svg items to react components actual svg code stays the same

this may be considered a breaking change as icons do not contain styles anymore and consumers have to apply them when they are needed

related PR: #5865

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 4, 2021
@armano2 armano2 marked this pull request as draft December 4, 2021 10:47
@@ -67,6 +67,8 @@

.expandSidebarButtonIcon {
transform: rotate(0);
/* todo: use variable */
color: #7a7a7a;
Copy link
Contributor Author

@armano2 armano2 Dec 4, 2021

Choose a reason for hiding this comment

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

previously color of this icon was hardcoded into component, and there was no way to change it without swizzling,

image image

not sure what value we should use

Copy link
Contributor Author

@armano2 armano2 Dec 5, 2021

Choose a reason for hiding this comment

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

any thoughts about this? we could actually remove this override and let control font color control how this icon looks

maybe we could use?

    color: var(--ifm-menu-color);

or we just keep it and do this in separate PR, as style for this is not yet in infima

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i created issue for this in infima facebookincubator/infima#193

@netlify
Copy link

netlify bot commented Dec 4, 2021

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 81881af
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63f6887176c2ba0008a60fa2
😎 Deploy Preview https://deploy-preview-6050--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@github-actions
Copy link

github-actions bot commented Dec 4, 2021

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🔴 43 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 78 🟢 96 🟢 100 🟢 100 🟢 90 Report

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Dec 4, 2021
@armano2 armano2 changed the title fix: save all svg icons as svg fix: Import icons as svg files instead of using react components Dec 5, 2021
@armano2 armano2 changed the title fix: Import icons as svg files instead of using react components fix: import icons as svg files instead of using react components Dec 5, 2021
@armano2 armano2 marked this pull request as ready for review December 5, 2021 15:21
@armano2
Copy link
Contributor Author

armano2 commented Dec 6, 2021

@Josh-Cena let me know if there is something else that i have to do in here

@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 6, 2021
@Josh-Cena
Copy link
Collaborator

I'm in favor of the current change. Just waiting for the input from @slorber

@Josh-Cena Josh-Cena changed the title fix: import icons as svg files instead of using react components refactor: import icons as svg files instead of using react components Dec 7, 2021
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks for working on this

This pr changes how svg files are stored in project and removes all css from them, this is first step to refactor how we import icons.

IMHO we should not do this in multiple steps, but in a single one, once we know exactly how we should handle the SVGs. This PR can serve as a POC to see if an approach is right.

This kind of change can be disruptive for our users with custom themes, and we should avoid doing 2 chances in 2 distinct releases when we can do only one.


The current approach is not good enough, the SVGs remain loaded through SVGR, converted to the same React components as before, and SVG markup is still duplicated in the final HTML

I don't know what is the best solution to this problem yet, just some ideas:

  • Use url('./icon.svg') in CSS => this should use file-loader instead of SVGR due to usage in CSS
  • Force usage of inline file-loader in JS file and use with <img> tag?

I asked a question here, some proposed approaches might be interesting to explore: https://twitter.com/sebastienlorber/status/1466024866491080707

Using SVG defs and SVG use also seems interesting, preferably if we can somehow lazy-load those refs instead of having a global registry

https://github.com/hponcede/use-svg-ssg

@armano2
Copy link
Contributor Author

armano2 commented Dec 8, 2021

personally i do not like icons in css, as there is no way to easily style them/color outside of creating new one

in my opinion sprites are way to go, i tested this change with https://github.com/JetBrains/svg-sprite-loader (or rather inspired version of it) and simple wrapper for react and it seem to work perfectly,

i still can use:

import arrow from './icons/arrow.svg';

return <arrow>

if thats a case icons are added automatically, withotu change of api, only difference is that component returned by this import is:

<svg viewBox="....">
  <use xlink:href="#arrow" />
</svg>

i could give it a try to implement it in non "mockup" version

@dsmmcken
Copy link
Contributor

dsmmcken commented Dec 9, 2021

personally i do not like icons in css, as there is no way to easily style them/color outside of creating new one

As long as they are one color icons, using mask for the svg, and background-color to set color makes it easy.

@Josh-Cena Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Jan 17, 2022
@armano2 armano2 closed this Apr 4, 2023
@armano2 armano2 deleted the use-svg-as-icons branch April 4, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants