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

Add SVG icon loader #18

Merged
merged 4 commits into from May 19, 2017

Conversation

Projects
None yet
3 participants
@zehfernandes
Member

zehfernandes commented May 17, 2017

I saw in the features list the item Generating icon font from SVGs

Have many articles about what is the best: "Icon Fonts vs. SVG sprite"

Personally, I prefer SVG and I think is the correct use of it. Symbols nodes with SVG sprite are more versatile, easy to align and very simple to maintain.

So, what this pull request does:

  • Create an SVG sprite using the loader svg-sprite-loader just with the SVG files in the icons folder
  • Icon component to insert the icon using the SVG file name as reference <Icon glyph='triangle' />
  • Add examples in the Dashboard page
@Stanko

This comment has been minimized.

Show comment
Hide comment
@Stanko

Stanko May 18, 2017

Member

@zehfernandes This is awesome, thank you! I totally agree this is better approach than icon fonts.

But I'll be a bit nitpicky, it seems like your IDE changed code formatting on few places. So I would really appreciate if you change it before we merge it.

Member

Stanko commented May 18, 2017

@zehfernandes This is awesome, thank you! I totally agree this is better approach than icon fonts.

But I'll be a bit nitpicky, it seems like your IDE changed code formatting on few places. So I would really appreciate if you change it before we merge it.

@Stanko

As I said in a previous comment, this is great stuff, but please change few formatting things.

Everything else 👍

Show outdated Hide outdated source/js/views/Dashboard/index.jsx
Show outdated Hide outdated source/js/views/Dashboard/index.jsx
Show outdated Hide outdated source/js/views/Dashboard/index.jsx
Show outdated Hide outdated source/js/views/Dashboard/index.jsx
Show outdated Hide outdated webpack.config.js
Show outdated Hide outdated webpack.config.js
Show outdated Hide outdated webpack.config.js
Show outdated Hide outdated webpack.config.js
@zehfernandes

This comment has been minimized.

Show comment
Hide comment
@zehfernandes

zehfernandes May 18, 2017

Member

@Stanko Sorry about the formatting changes.
I supposed the prettier plugin was using the eslint's rules to format. But not :(

My last commit fixed that. 👍

Member

zehfernandes commented May 18, 2017

@Stanko Sorry about the formatting changes.
I supposed the prettier plugin was using the eslint's rules to format. But not :(

My last commit fixed that. 👍

@oliverdore-work

This comment has been minimized.

Show comment
Hide comment
@oliverdore-work

oliverdore-work May 18, 2017

Member

@zehfernandes I made that assumption about Prettier and ESLint recently too. Easily done.

Member

oliverdore-work commented May 18, 2017

@zehfernandes I made that assumption about Prettier and ESLint recently too. Easily done.

@Stanko

Stanko approved these changes May 19, 2017

@Stanko Stanko merged commit a405503 into workco:master May 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment