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

Fix simple icons color fill #194

Merged
merged 2 commits into from
Aug 31, 2021
Merged

Fix simple icons color fill #194

merged 2 commits into from
Aug 31, 2021

Conversation

dangilbert
Copy link

@dangilbert dangilbert commented Aug 31, 2021

dangilbert /master → Lissy93/dashy Commits: 2 | Files Changed: 3 | Additions: 13 dangilbert Powered by Pull Request Badge

Category:
One of: Bugfix

Overview
I was having problems with the SimpleIcons applying a color tint because the icons being loaded as SVGs via URL were wrapped in an object thus not allowing css styling to apply.

By including the node package for simple-icons we can directly access the SVG and PATH in order to be able to set the fill to currentColor

New Vars (if applicable)
Dependency added on simple-icons:^5.12.0

Screenshot (if applicable)
Before:
Screenshot 2021-08-31 at 13 03 51
After:
Screenshot 2021-08-31 at 13 03 55

Code Quality Checklist (Please complete)

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors
  • (If a new config option is added) Attribute is outlined in the schema and documented
  • (If a new dependency is added) Package is essential, and has been checked out for security or performance
  • Bumps version, if new feature added

@dangilbert dangilbert requested a review from Lissy93 as a code owner August 31, 2021 10:38
@netlify
Copy link

netlify bot commented Aug 31, 2021

✔️ Deploy Preview for dashy-dev ready!

🔨 Explore the source changes: c9fc7e4

🔍 Inspect the deploy log: https://app.netlify.com/sites/dashy-dev/deploys/612e0b4192dceb00079ae165

😎 Browse the preview: https://deploy-preview-194--dashy-dev.netlify.app/

@viezly
Copy link

viezly bot commented Aug 31, 2021

Changes preview:

👀 Review pull request on Viezly

@Lissy93
Copy link
Owner

Lissy93 commented Aug 31, 2021

Looking good!
I guess the main reason I didn't use the simple-icons package in the first place, is because it's 10.5 MB

@dangilbert
Copy link
Author

Looks like the deploy worked and then git had a failure.

And also the yarn lock change task looks like it's failing due to this:
Simek/yarn-lock-changes#26

@dangilbert
Copy link
Author

Looking good!
I guess the main reason I didn't use the simple-icons package in the first place, is because it's 10.5 MB

Yeah, that makes sense. I couldn't find another way of altering the color of the simple icons that didn't involve using javascript to inject into the object tags, which felt a bit more convoluted than it needed to be

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

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

Just 1 small change

src/components/LinkItems/ItemIcon.vue Outdated Show resolved Hide resolved
@Lissy93
Copy link
Owner

Lissy93 commented Aug 31, 2021

Yeah I think the failure in the CI is a permissions thing, it works just fine for me locally so it should be all okay

@dangilbert dangilbert requested a review from Lissy93 August 31, 2021 11:04
@Lissy93
Copy link
Owner

Lissy93 commented Aug 31, 2021

Good fix- thank you for implementing :)
I'm going to merge it, but monitor performance because of the packages size. If it has any noticable impact on the app when Simple Icons are not being used, then I might need to find a different approach.

@Lissy93 Lissy93 merged commit 29c2e34 into Lissy93:master Aug 31, 2021
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.

3 participants