Skip to content

Add social icons to Icon component#124

Merged
nvmusoke merged 5 commits intomasterfrom
icons
Nov 18, 2019
Merged

Add social icons to Icon component#124
nvmusoke merged 5 commits intomasterfrom
icons

Conversation

@nvmusoke
Copy link
Copy Markdown
Contributor

updated existing icons & created new icons using icons from designer provided folder.

@mdespuits
Copy link
Copy Markdown
Contributor

Before I look closely at the existing Icon components, it's worth mentioning that this may be a good opportunity to talk about using svgo as a tool to optimize our SVG icons: https://github.com/svg/svgo. Just browsing through some of these I'm fairly sure incorporating some optimization will be a good practice and also possible to add to testing/committing workflow.

@chelshaw @pixelbandito thoughts?

Comment thread src/components/Icon/icons/Twitter.js Outdated
@pixelbandito
Copy link
Copy Markdown
Contributor

@nvmusoke I'd be happy to approve this, but I agree that you should run those icons through svgo first. I remember it being pretty easy, but let me or @mdespuits know if you need a hand.

@nvmusoke
Copy link
Copy Markdown
Contributor Author

@mdespuits @pixelbandito yeah i'm gonna run them through svgo. I tested it in a new branch and it was really simple.

Comment thread src/components/Icon/icons/Google.js Outdated
Comment thread src/components/Icon/icons/Google.js Outdated
Comment thread src/components/Icon/icons/Reddit.js Outdated
Copy link
Copy Markdown
Contributor

@chelshaw chelshaw left a comment

Choose a reason for hiding this comment

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

A few updates

What are all the files in /Icon/main for?

Comment thread src/components/Icon/icons/Slack.js Outdated
@nvmusoke
Copy link
Copy Markdown
Contributor Author

nvmusoke commented Nov 4, 2019

A few updates

What are all the files in /Icon/main for?

All the svgs i've yet to make js files for. I initially added them because i thought I needed them, but then I thought it would be easier to keep them in the same project.

@pixelbandito
Copy link
Copy Markdown
Contributor

My initial preference is not to keep working .svg files in the codebase, but I can see how it's a helpful way to keep track of what's left to do.

I'm ok with it.

@nvmusoke nvmusoke requested a review from chelshaw November 12, 2019 20:35
@nvmusoke nvmusoke dismissed chelshaw’s stale review November 18, 2019 16:44

other devs seem fine with keeping svg files in.

@nvmusoke nvmusoke merged commit 8fba87e into master Nov 18, 2019
@nvmusoke nvmusoke deleted the icons branch November 18, 2019 16:49
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.

4 participants