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

Consider using SVGR to simplify SVG handling #14628

Open
gziolo opened this issue Mar 26, 2019 · 9 comments

Comments

@gziolo
Copy link
Member

@gziolo gziolo commented Mar 26, 2019

This idea comes from the chat with @mor10 in #14590 where he shared how one can extend default webpack config with custom loaders. It sparked my interest as we still can improve our workflow around using SVG files. To bridge ReactDOM and React Native we introduce wrapper component for all used SVG tag types in #9685. However, it would be great to be able to use regular SVG files without any manual modification and let Babel (to work with npm packages out of the box in contrast to webpack) do the trick.

My initial reaction from #14590 (comment):

I was looking into https://github.com/smooth-code/svgr and I wasn't aware it has so many useful packages in addition to CLI tool and webpack loader. It makes me wonder if we shouldn't redefine the way we handle SVG icons and maybe start using files with SVGs and tackle them with custom Babel loader which would convert them to React components on the fly providing a unified format with both React web and React Native can process.

This is what @mor10 shared in #14590 (comment):

I would back the inclusion of SVGR 100%. It makes adding and using SVGs in blocks significantly easier and removes the need for custom workarounds. Here's how I'm using it to add a block logo as a React component:

import customLogoURL, { ReactComponent as customLogo } from "./logo.svg";

registerBlockType("block/myblock", {
  title: __("A Block", "myblocs"),
  icon: {
    src: customLogo // The React component
  },
  category: "regular",
  attributes: {
    blockImage: {
      type: "string",
      default: customLogoURL // The URL to the original SVG
    }
  },
  (...)
}

I did a quick check on the SVGR monorepo and found some useful packages which could work as a good example of how we could customize things:

@gziolo

This comment has been minimized.

Copy link
Member Author

@gziolo gziolo commented Apr 1, 2019

Extracting SVG icons in #14743 might help to explore how this could be achieved as all SVG based icons would be located in their own files making it easier to reason how to convert them to the current form from the commonly known file format.

@mkaz

This comment has been minimized.

Copy link
Member

@mkaz mkaz commented Oct 14, 2019

@gziolo Any updates or new idea/approaches for this one? How can I help it along?

We're looking at improving how social icons uses and manages SVG, right now it requires duplication on PHP and JS sides, which we would be better if we don't need to.

@gziolo

This comment has been minimized.

Copy link
Member Author

@gziolo gziolo commented Oct 15, 2019

@gziolo Any updates or new idea/approaches for this one? How can I help it along?

I didn't perform any further exploration. I documented all my findings above. Feel free to check other options, as well. I'm happy to discuss all the technical options.

We're looking at improving how social icons uses and manages SVG, right now it requires duplication on PHP and JS sides, which we would be better if we don't need to.

I saw those efforts. I think the biggest challenge there is how to store SVG with the save method. I don't quite know why you decide to use render_callback in the first place so if you could share more details it would allow other folks to get involved, too. So to conclude, we need to know all the requirements in regard to working with blocks that use SVG icons.

Originally, I was mostly concerned about all SVG icons we use with UI components rendered in the block editor and I didn't consider putting them in the post content. However, it looks like we should cover both cases to make it work as intended.

@mkaz

This comment has been minimized.

Copy link
Member

@mkaz mkaz commented Oct 16, 2019

For social links the render_callback was used to allow for SVG to be authored in a post by a user without Admin or Editor rights due to KSES. If the save method was used in Editor/JS side than an author would not be able to use the block.

Also, using render_callback allows for changing the SVG markup as logos change over time without having to reauthor the posts spots that include the blocks.

The downside for using render_callback was all the SVGs had to be loadable via PHP, and at that time, not included as their own svg files, thus the "weird" include and definition within PHP.

Now, @jasmussen is exploring a solution to use CSS background which would allow using individual SVG files. This would actually solve the server side rendering part too, since SVG markup would not be included in the post_content. However, we still need to load the SVG so they can be a block icon, so would be great if we could use webpack svg loader or something similar to import a standard svg file as a react component.

@jasmussen

This comment has been minimized.

Copy link
Contributor

@jasmussen jasmussen commented Oct 16, 2019

I saw those efforts. I think the biggest challenge there is how to store SVG with the save method. I don't quite know why you decide to use render_callback in the first place so if you could share more details it would allow other folks to get involved, too. So to conclude, we need to know all the requirements in regard to working with blocks that use SVG icons.

I took a new approach that would allow us to use completely external SVG files. You can see it here:

https://codepen.io/joen/pen/XWWXvBL

All of that works and is functional, and we can update Social Links to use this method instead, and it would solve the problems Marcus is outlining.

The only missing pieces is icons in the block library:

Screenshot 2019-10-16 at 09 46 06

It would be so nice if those icons could use the same SVG files as we load using the above codepen-outlined method.

@gziolo

This comment has been minimized.

Copy link
Member Author

@gziolo gziolo commented Oct 16, 2019

It would be so nice if those icons could use the same SVG files as we load using the above codepen-outlined method.

Can you elaborate what are the benefits of inlining SVG files into JavaScript code rather than offer a way to use CSS-based icons fir blocks as well? The benefit is that you wouldn’t have to download the same icon twice. The drawback is that you probably can’t use HTML based queries for SVG tags.

@gziolo

This comment has been minimized.

Copy link
Member Author

@gziolo gziolo commented Oct 17, 2019

There is a related discussion on how to make it possible to inline SVGs and other assets in create-react-app: facebook/create-react-app#3722. They are seeking for the solution which doesn’t require to rewrite Webpack config each time you want to register a new asset type and rather provide ways which scale well with Babel.

There is still no decision made as it’s a complex issue to solve. They are talking Babel macros which I’m exploring in #16088 for the block.json files. The biggest challenge is that it breaks development workflows because all tooling assumes that you operate on JS files so in both cases JSON and SVG, the inlined files get cached and Babel don’t clear the cache when those files change. This need to be solved on Babel side first, the only known workaround so far is to disable caching for the files which import those assets...

The existing integration for SVG in create-react-app is done with webpack: facebook/create-react-app#3718.

@jasmussen

This comment has been minimized.

Copy link
Contributor

@jasmussen jasmussen commented Oct 17, 2019

Can you elaborate what are the benefits of inlining SVG files into JavaScript code rather than offer a way to use CSS-based icons fir blocks as well? The benefit is that you wouldn’t have to download the same icon twice. The drawback is that you probably can’t use HTML based queries for SVG tags.

To be clear I'm not personally suggesting any specific technical implementation. To me, the goal here isn't SVGR. The specific challenge I'm pondering for Social Links is: how do we reduce duplicate code? If you look at https://codepen.io/joen/pen/XWWXvBL, you'll see that each logo is referenced like so:

background: url(https://cldup.com/w4j97g2E1Y.svg) no-repeat center center;

https://cldup.com/w4j97g2E1Y.svg is an actual URL, and as you see it's just a black SVG. The CSS in the codepen re-colors that SVG to be the color of the specific brand. This already works in the editor and the frontend. The only missing piece is showing those same icons in the block library. And I can't think of a simple way to use an external URL such as https://cldup.com/w4j97g2E1Y.svg to change icons in the block library, and re-color them, without using something like SVGR. But that's the goal and if there are other options, that's cool.

I would personally LOVE if we could do this:

import Icon from './icon.svg';

That would solve it 🤘

@gziolo

This comment has been minimized.

Copy link
Member Author

@gziolo gziolo commented Oct 26, 2019

I’m sharing a related article about cross-platform SVG icons:
http://nicolasgallagher.com/making-svg-icon-libraries-for-react-apps/

@mkaz mkaz referenced this issue Nov 1, 2019
0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.