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

VUE-223 KvIcon: dynamically inline most icons, sprite common icons #1555

Merged
merged 6 commits into from
Mar 31, 2020

Conversation

ryan-ludwig
Copy link
Contributor

Follow up from #1541

This PR moves the majority of icon files out of the global icon sprite and into a folder where we dynamically import them only in the components which use them. This dramatically reduces the DOM nodes on all pages from ~465 to ~25 from the sprite.

  • Icons which are used on most pages are kept in the sprite.
  • Icons which I couldn't find a reference to in the UI repo have been removed.
  • Some folder reorganization
  • While testing icons if I noticed a <span or similar with an @click event on it I changed it to a <button for accessibility purposes
  • After struggling with several icons not working correctly with webpack-svgstore-plugin which makes the sprite I ended up forking it and updating the SVGO dependency. That seems to have solved the issues. I have a PR up for them, hopefully it'll be resolved soon.

To test:
pull down,
run npm install,
browse to some pages you've worked on, make sure icons look correct.
npm run storybook to see the available icons

package.json Outdated
@@ -180,7 +180,7 @@
"webpack-filter-warnings-plugin": "^1.2.1",
"webpack-hot-middleware": "^2.25.0",
"webpack-node-externals": "^1.7.2",
"webpack-svgstore-plugin": "^4.1.0"
"webpack-svgstore-plugin": "ryan-ludwig/webpack-svgstore-plugin#develop"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unfortunate, but the only way I could get the sprite plugin to work with our magnifying glass icon was to fork it and update it's SVGO dependency. I have a PR up with them.

I believe @BoulderBrains has had problems adding SVGs to the sprite in the past, I believe this is the same issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to that PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@eddieferrer eddieferrer Mar 18, 2020

Choose a reason for hiding this comment

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

I think webpack-svgstore-plugin is practically abandoned. We should try to replace it with something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've wondered if we shouldn't just do the sprite manually since it has so few items now.

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 have my doubts that webpack-svgstore-plugin is going to merge my PR. Don't really love that this is pointing to my personal github for something we're wanting to get into prod.

2 options I think:

  1. Fork webpack-svgstore-plugin to the kiva github, make the same change, point package.json to that.
  2. Ditch webpack-svgstore-plugin, roll the sprite by hand (easy), and ajax it in from TheHeader.vue or something similar.

Both have upsides and downsides. @emuvente @mcstover thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with #1.

Alternatively, if we take the #2 approach could we just import and render the image in App.vue to ensure it's always present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to run with #1 for today to get this big PR into dev.

With #2, yeah I think putting the sprite directly into App.vue makes sense to me 👍.

@eddieferrer
Copy link
Contributor

Lets also please make sure to squash these commits when we merge this in. Thats a lot of commits...

@ryan-ludwig
Copy link
Contributor Author

ryan-ludwig commented Mar 18, 2020

Lets also please make sure to squash these commits when we merge this in. Thats a lot of commits...

Good call, I'll rebase after reviews.
Edit: done.

Run all icons through SVGO to tidy things up

Move thanks page social share icons into kv-icon

Update print and question mark icons on thanks page

Move credit card icons for BraintreeCheckout

Move lock to an inline-icon

Allow KvIcon2 to pull form the sprite or inline depending on prop.

Move close x to sprited icon

Move notice icon over to kvicon2

Move all of the lending stats icons out of the sprite

Tweak how icons fill their space

Create a logo folder, move Kiva logo into it.

Move dedicate-heart icon out of the sprite

Remove unused checkbox icons

Move KvTipMessage icons out of sprite

Update Stats Section completed icon

Add a max width to icons

Move confirmation checkbox icon over to kv-icon2

Move DonationItem icons to kv-icon2

Pull the x icon from sprite in kv-tip-message

Use close and magnify glass from sprite in header

KvFacebookButton use kv-icon2

More triangle icon conversions

Magnify glass from sprite

Triangles from sprite in kv-pagination

Convert viewtoggle icons to inline

Use star from sprite

Fix height

Rename confirmation icon to checkmark-in-circle

Update checkoutnow button, remove checkmark-in-circle icon.

Couple of small-x icon updates

Convert order totals icon. Change from a tags to button tags for semantics.

Update loan card close icons

Update specific promo banners (although they could likely be deleted)

Move over chevrons

Reorganize icon folders. Remove unused icons.

Convert promo banner icons

Update genericpromobanner icons to match contentful. Rename a few icons.

Rename KvIcon2 to KvIcon

Color dedicate-heart using scss

Add max-height

Fix icon location

Change tags to buttons for accessibility
@ryan-ludwig ryan-ludwig force-pushed the VUE-223_kv-icon-slimsprite branch from 4e372b7 to 298d1e0 Compare March 19, 2020 16:18
Copy link
Contributor

@BoulderBrains BoulderBrains left a comment

Choose a reason for hiding this comment

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

🧙‍♂️

@mcstover
Copy link
Collaborator

@ryan-ludwig @emuvente Please hold on merging this until next week.

Copy link
Collaborator

@emuvente emuvente left a comment

Choose a reason for hiding this comment

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

👍 love this

@ryan-ludwig ryan-ludwig merged commit 08bb146 into master Mar 31, 2020
@ryan-ludwig ryan-ludwig deleted the VUE-223_kv-icon-slimsprite branch March 31, 2020 16:51
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.

5 participants