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

Added boardgamegeek icon #230

Closed
wants to merge 40 commits into from
Closed

Added boardgamegeek icon #230

wants to merge 40 commits into from

Conversation

diondresschers
Copy link
Contributor

Hi, great fork! 🥇 I've added this due to: Icon Request: icon-boardgamegeek #228.

@diondresschers
Copy link
Contributor Author

Icon request: Nordcast icon #227

@diondresschers diondresschers mentioned this pull request Apr 2, 2020
@whoabuddy
Copy link

Thank you for including the Blockstack icon with this update! Much appreciated!!

@xuv
Copy link
Member

xuv commented Jun 7, 2020

Hello @diondresschers

This is a very big PR. Do you mind if we break it down a little. There are a few things that I would definitely like to add to the set. But other icons that might need some more work.

So far, as a general recommendation, I see that a bunch of icons with an "open design" don't really work well with the rest of the set. The contour lines are too thin compared to the contour lines being used in previous icons. Take a look at the fa-rocket-o you've included here and the plus-square-o we already have in the set.

I think it's worth-while redesigning the fa-rocket-o to be more in line with the rest.

As for the bunny series. I think it's a bit too much to include 4 of them. I would only keep one. I particularly like the simplicity in the design of the fa-bunny-back. I remove the others. And rename this one as fa-bunny.

Another thing that is difficult to balance, I also question myself al lot regarding this, is the relative sizer for the different brand icons. I like to keep sizes relatively similar between all of them. It's a difficult exercises as some brands have complicated designs and they don't read very well at small sizes. Some others have very bold shapes that seem to stick out more than others. With that in mind, I think that weblate, speaker-deck, webmention and r-project could be reduced in size to fit better with the rest of the brand set. I'm hesitating for vuejs. It could also be a little smaller, if the thin line in the design doesn't disappear completely at small sizes.

Those are my suggestions. Let me know what you think. And if you need help to make those changes.

Thank you for your help.

Julien

@diondresschers
Copy link
Contributor Author

Hi Julien, the PRs were made over several days, so I think at the end it looked like one BIG PR. Sure it's fine to break it down. I will work next days to resolve all individual issues. If there are questions I will let you know. I hope you have the time to resolve them. I think ForkAwesome is a beautiful project and I would hate to see it die...

@xuv
Copy link
Member

xuv commented Jun 7, 2020

It's my fault. I should have picked this up earlier. When you started the PR. Thank you for resolving the individual issues. I think I'll bump the minor version number with all this. I've been meaning to do this for a while. Just bumping the patch number has not been a good idea from the beginning.

I'm also leaning how to maintain an open source project. So I still make mistake. I agree with you. This project needs to stay alive. Let's go through this together see how we can have a smoother collaborative process afterwards.

@xuv
Copy link
Member

xuv commented Jun 15, 2020

hello @diondresschers

Let me know how it goes. If it’s too much of a trouble to make all those changes at once, we can think of an alternative.

@diondresschers
Copy link
Contributor Author

Hi @xuv

It would be possible (and for me the easiest) to make one big commit

If you are okay with that, I will create 1 BIG commit, leaving out the:

fa-rocket-o
weblate
speaker-deck
webmention
r-project
vuejs
duckduckgo

and will rename fa-bunny-back to fa-bunny (and kill the other bunnies).

If you then can successfully merge my commit with Master Branch, that would be awesome.

I can than start to redo the earlier mentioned icons in smaller PRs, one by one...

Let me know if that sounds okay with, so we can move on.

@xuv
Copy link
Member

xuv commented Jun 16, 2020

Sure. Let’s do that.

@diondresschers
Copy link
Contributor Author

Okay @xuv

I've deleted the Fork in my own GitHub Repo and created a brand new Fork so all possible conflicts are gone. Than I've copied all the required/wanted SVG icons from my own 'old' repo-backup to the new fresh Fork, and updated the icons.yml file.

I've created a single (big) pull request with all the new icons:
#249

@xuv
Copy link
Member

xuv commented Jun 24, 2020

Closing this as most of it was merged in a later PR.

@xuv xuv closed this Jun 24, 2020
@antenore
Copy link

@xuv the commit 5fb6ecf hasn't been merged and I think that many other commits were not included.

My main burden is about the weblate icon, I miss it really hard. See #218 but I think others have the same issue?

Can you kindly cherry-pick the above commit ?

Thanks in advance!!!

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.

None yet

4 participants