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

Optimize SVG assets and update builder interface #5937

Merged

Conversation

joshblack
Copy link
Contributor

@joshblack joshblack commented Apr 24, 2020

Closes #5568

This PR adds in the ability to optimize SVG assets in the output package directory, alongside including QOL fixes for our builder packages.

This PR also adds in tests for the icons-vue package since we ended up needing to update the builder with the new metadata format. A big source of the LOC changes comes from the snapshots generating its public API.

Changelog

New

  • e2e/icons-vue
    • Add PublicAPI tests for icons-vue
  • Add official vue builder to icon-build-helpers
  • Add svg builder for optimizing SVG assets
  • Add new output extension, this extension does the following:
    • Optimizes asset sources
    • Transform asset source to JS descriptor
    • Provides module name

Changed

  • svg assets now live in src/svg instead of svg
  • icon-build-helpers
    • Metadata now takes in an updated input option for SVG and extension directories
      • Update e2e tests to use new syntax for input
      • Update sketch task to use new syntax
    • Builders are now simplified with a new metadata structure
      • react builder has been updated
      • vanilla builder has been updated
    • Bugfix for metadata extensions where it was called with an empty object instead of letting default options be applied
  • Update all icon/pictogram tasks with new metadata syntax

Removed

  • Un-used files from icon-build-helpers package
  • Remove old entrypoint test for icons-vue
  • Remove unused icons-vue task files

Testing / Reviewing

@netlify
Copy link

netlify bot commented Apr 24, 2020

Deploy preview for carbon-components-react ready!

Built with commit 17c1775

https://deploy-preview-5937--carbon-components-react.netlify.app

@joshblack joshblack marked this pull request as ready for review April 24, 2020 20:09
@joshblack joshblack requested review from laurenmrice and a team as code owners April 24, 2020 20:09
@ghost ghost requested review from asudoh and emyarod April 24, 2020 20:09
@joshblack
Copy link
Contributor Author

@laurenmrice just a heads up for changes, this is moving the SVG files from svg to src/svg 👀

@netlify
Copy link

netlify bot commented Apr 24, 2020

Deploy preview for carbon-elements failed.

Built with commit 91b54bd

https://app.netlify.com/sites/carbon-elements/deploys/5ea335925086230007c15e36

@netlify
Copy link

netlify bot commented Apr 24, 2020

Deploy preview for carbon-elements ready!

Built with commit 17c1775

https://deploy-preview-5937--carbon-elements.netlify.app

@joshblack
Copy link
Contributor Author

FYI @davidicus this is our PR which updates the entrypoints for the @carbon/icons-react package 👀

Instead of the flat entrypoint it should now re-export so the sideEffects flag works as intended.

Let me know if you want to pair up and try it out in a product or on the IoT library 👀

Copy link
Member

@emyarod emyarod 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

@joshblack
Copy link
Contributor Author

@asudoh let me know if there is anything else I can do / write-up to help with reviews!

@asudoh
Copy link
Contributor

asudoh commented Apr 28, 2020

Some quick findings:

  • Seems that @carbon/icons/{es,lib}/{{iconName}}/index.js is gone - Is it intentional? If so, do we need a backward-compatibility layer?
  • Seems that @carbon/icons/svg/* is moved to another directory, though svg is in the files list of @carbon/icons package. Is it intentional?

@joshblack
Copy link
Contributor Author

joshblack commented Apr 29, 2020

Hey @asudoh! 👋

Seems that @carbon/icons/{es,lib}/{{iconName}}/index.js is gone - Is it intentional?

I don't believe index.js files are in @carbon/icons presently IIRC 🤔 was trying to go back a couple of versions but can't seem to find index.js per-icon

Seems that @carbon/icons/svg/* is moved to another directory, though svg is in the files list of @carbon/icons package. Is it intentional?

Yeah! Source files are moved over into src/svg but now there will be an optimized svg folder at /svg for folks to use 👀

@asudoh
Copy link
Contributor

asudoh commented Apr 29, 2020

I don't believe index.js files are in @carbon/icons presently IIRC 🤔 was trying to go back a couple of versions but can't seem to find index.js per-icon

Just in case it was not clear enough, I was referring to: https://unpkg.com/browse/@carbon/icons@10.10.2/es/chevron--down/index.js

Yeah! Source files are moved over into src/svg but now there will be an optimized svg folder at /svg for folks to use 👀

Sounds good, thank you for clarifying.

@joshblack
Copy link
Contributor Author

@asudoh nice! Got it, thanks for clarifying 🙏 Will make sure to update glyph.js -> index.js 👀

@joshblack
Copy link
Contributor Author

Should be updated now!

@joshblack joshblack merged commit ffcc615 into carbon-design-system:master Apr 29, 2020
@joshblack joshblack deleted the feat/optimize-svg-take-2 branch April 29, 2020 14:07
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.

Icon bug: Using information--filled and closed icons together causes the info icon to render incorrectly
4 participants