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

Adds a new icon set named Fontisto (http://fontisto.com) #977

Merged
merged 5 commits into from
Jun 7, 2019

Conversation

rfbezerra
Copy link
Contributor

Recently I found a good icon-set named Fontisto
This PR adds it to the react-native-vector-icons icon sets

@catalinmiron
Copy link
Collaborator

@rfbezerra this is a great PR but you're also doing many other things. Could you please extract the version bumping to a separate PR (that can fix and have a positive impact over existing issues: #993, #972 )

@rfbezerra
Copy link
Contributor Author

Oh, my bad.
I didn't realize this version bump.
I'll fix and push again.

@hampustagerud
Copy link
Collaborator

You have included old commits somehow, they have incorrect hashes and will mess with existing commits. Could you fix them as well? 🙂

@rfbezerra
Copy link
Contributor Author

I don't know what happened before, but I think this is ok now.
Can you check now?

Copy link
Collaborator

@catalinmiron catalinmiron left a comment

Choose a reason for hiding this comment

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

LGTM!
PS: Fontisto is really looking great!

cc: @hampustagerud @oblador

@hampustagerud
Copy link
Collaborator

Indeed, this font is really nice! The code looks fine and there is a build script as well 👍 Only thing is, could you add it to the directory and IconExplorer project as well? 🙂

@catalinmiron
Copy link
Collaborator

catalinmiron commented Jun 5, 2019

@rfbezerra since I don't have push access to your fork, I've cloned it and did the work. Here's the diff that you can apply:
https://drive.google.com/file/d/1DU4O6nY1IAZ5IF-aySabGP6cyrbPYI1g/view?usp=sharing

git apply {document}.md

Please do let me know if you need any help with this. Thanks!

@rfbezerra
Copy link
Contributor Author

I updated sources.
Did you know why every time I run yarn to install dependencies, all integrity fields from yarn.lock is deleted. I'm using node v10.6.0 with yarn v1.9.4.

@catalinmiron
Copy link
Collaborator

@rfbezerra I don't know about the yarn.lock. :(
You forgot to add Fontisto to Examples/IconExplorer plist.

My workflow on doing is npm link (root directory) -> npm link react-native-vector-icons (Examples/IconExplorer) -> react-native link.

@@ -158,7 +160,7 @@ class App extends PureComponent {
let familyName = family;

if (family === 'FontAwesome5') {
if (FontAwesome5Meta['solid'].indexOf(name) === -1)
if (FontAwesome5Meta.solid.indexOf(name) === -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 🙂 Thanks!

@hampustagerud
Copy link
Collaborator

Only thing left that I can find is that the font is not added to the IconExplorer ios project, I'd do it myself but I am currently not near a computer 🙂 Maybe linking the app should be enough but don't take my word for it.

@rfbezerra
Copy link
Contributor Author

I test only in Android, my bad.
I added through react-native link
Seem to be good now

@catalinmiron
Copy link
Collaborator

🥳🥳 LGTM
@hampustagerud could you please take another look before merging it?
Thanks

@hampustagerud hampustagerud merged commit 10a0cdf into oblador:master Jun 7, 2019
@hampustagerud
Copy link
Collaborator

Thank you both 🙂

@catalinmiron
Copy link
Collaborator

🤗 🍾 🎉

@rfbezerra rfbezerra deleted the add_fontisto_icon_set branch June 7, 2019 14:23
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