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

Add Vue.js Icon #358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Vue.js Icon #358

wants to merge 1 commit into from

Conversation

mrdrogdrog
Copy link
Contributor

This PR adds the vue.js logo from vuejs/art#12

Fixes #172

Copy link
Member

@shinenelson shinenelson left a comment

Choose a reason for hiding this comment

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

image

Please re-base with the head branch.


Also, when I built the icon locally, there was a white-space removal change at the end of <path>

- 639.98761,1006.8369 Z" />
+ 639.98761,1006.8369 Z"/>

Do you have the same change?

@mrdrogdrog
Copy link
Contributor Author

mrdrogdrog commented Sep 5, 2021

fixed it. (to make things a bit easier for me I rebased this branch on #354)

Copy link
Member

@shinenelson shinenelson left a comment

Choose a reason for hiding this comment

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

I had this feeling that something was off about this logo when I first built this pull request.

Now when I was looking at the id, it occurred to me that I could check with Font Awesome. Font Awesome has the vuejs logo where the dark and alpha are inverted ( from this pull request ).

image

It led me to wonder to check again with the actual Vue.js logo on the project website. And then the Font Awesome version made more sense because the darker color in the logo is in the top of the logo and not along the logo. ( Personally, I prefer the Font Awesome representation ).

But considering this version was accepted by the upstream project, I am not sure how it feels about Font Awesome's representation of the brand. Ping @yyx990803 to get feedback on how the Vue.js project feels about this.

@@ -7889,6 +7889,14 @@ icons:
categories:
- Brand Icons

- name: Vue.js
id: vue-js
Copy link
Member

Choose a reason for hiding this comment

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

let us use an id that is requested in the icon request issue, i.e, fa-vue / fa-vuejs. Maybe we could also set up an alias for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the name of the lib is "vue.JS" I chose fa-vuejs.

Copy link
Member

Choose a reason for hiding this comment

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

If you would like, you could add an aliases key for the icon in src/icons/icons.yml, but that is not a requirement though.

src/icons/svg/vue.svg Outdated Show resolved Hide resolved
Copy link
Member

@shinenelson shinenelson left a comment

Choose a reason for hiding this comment

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

I am going to leave this open for a while to see if we can get direct feedback from the Vue.js project regarding the black and alpha that I raised in my previous review comment ( after comparing with Font Awesome ).

@DougInAMug DougInAMug added the icon-request New icon request label Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
icon-request New icon request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Icon request: fa-vue
3 participants