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 functionality to specify custom icons #114

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

kx-chen
Copy link
Contributor

@kx-chen kx-chen commented Oct 27, 2018

Add functionality to specify custom icons, either from an image or class, based on values set in websites.yml

Closes #110

In the websites.yml file, people can now specify if they would like an image or a class to be used for the icon of a website. Keybase's was added as an example. By specifying src : ... it will construct an image tag, and by specifying class: ... it will construct an i tag.

This code desperately needs a second pair of eyes to look at it and suggest feedback!

websites.yml Outdated
@@ -144,3 +144,6 @@ avatar:
tumblr: opengraph
twitter:
url: https://twitter.com/{u}/profile_image?size=original
icon_css:
keybase:
src: "https://png.icons8.com/ios/30/000000/keybase.png"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are quotes necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. My bad. They have been removed.

@kx-chen kx-chen force-pushed the custom-icons branch 2 times, most recently from 34c26dd to 42ec582 Compare October 28, 2018 01:18
websites.yml Outdated
@@ -144,3 +144,6 @@ avatar:
tumblr: opengraph
twitter:
url: https://twitter.com/{u}/profile_image?size=original
icon_css:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this icon_css: structure / design is not declarative enough. It describes the implementation.

The purpose of this file is to remove any 'meaning' and just include re-usable facts.

The 'bug' at the moment is that the website key used throughout this file is also the fontawesome name.

Which isnt good enough, all the time.

So we need a new section of the yaml which provides facts about logos for the website.

logos:
  wikimedia:
    fontawesome: wikipedia-fa
  keybase:
    url: https://png.icons8.com/ios/30/000000/keybase.png

Copy link
Collaborator

@seeeturtle seeeturtle Oct 28, 2018

Choose a reason for hiding this comment

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

I think the title should be "declare logo information" rather than "adding functionality to customize".

Since this can be used not only in icons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jayvdb Thanks. So to my understanding, the keys in the yml should not describe the implementation of the icons (src/classes in html) but should instead describe facts about the icon (eg: font awesome icon name and url of icon).

I think that making a function in the javascript to determine the implementation (src or class) of the icon based on its key in the yml should work.

I also think it'll a good idea to keep having the fontawesome name determined by the website's name by default. This way we would not have to re declare each website icon's fontawesome name manually. When the fontawesome name does differ from the website's name, they can simply specify the font awesome name in the yml file. Thoughts?


function determineHtmlElement(htmlAttribute) {
var elementName;
if(htmlAttribute == 'src') {

Choose a reason for hiding this comment

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

It's always a good habit to use === instead of == :P

Choose a reason for hiding this comment

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

Also add a space before (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. A space before the if or a space before the function?

@ksdme
Copy link

ksdme commented Oct 28, 2018

I am not sure if this repo follows any commit guidelines, but your commit message looks awfully long. Consider breaking it into two lines, 80 is a good character limit for a single line.

@kx-chen kx-chen force-pushed the custom-icons branch 5 times, most recently from 7066fff to 01df69a Compare October 28, 2018 20:06
Copy link
Contributor

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Anything else @jayvdb @li-boxuan @ksdme?

Copy link

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@wisn wisn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -144,3 +144,6 @@ avatar:
tumblr: opengraph
twitter:
url: https://twitter.com/{u}/profile_image?size=original
logos:
keybase:
url: https://png.icons8.com/ios/30/000000/keybase.png
Copy link

@bekicot bekicot Oct 29, 2018

Choose a reason for hiding this comment

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

Should add an example for font awesome usage. Like is it full fa-circle or just the corresponding icon like circle. I would recommend implementing the earlier, for further decoration. The drawback is you have to put vendor prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a website which needs the fontawesome: key?

If not, cant be added, but it looks like it should work, and the next student can do the testing or fix it if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, pretty much all of the sites are currently using fontawesome. I added the fontawesome key in case the name of the site differed from the fontawesome name. Currently, the code defaults to using the website name as the fontawesome key.

To answer @bekicot 's question: it should be the full fontawesome name. For instance, strava should be fab fa-strava. Sometimes, the prefix can be something other than fab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add an example and address the other suggestions in a little bit.

return element;
}

function determineHtmlAttribute(key) {
Copy link

Choose a reason for hiding this comment

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

Should have more descriptive name (include logo in the name of the function), or add comment to explain what is the function for.

return logoElement;
}

function determineHtmlElement(htmlAttribute) {
Copy link

Choose a reason for hiding this comment

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

Should have more descriptive name (include logo in the name of the function), or add comment to explain what is the function for.

@kx-chen kx-chen force-pushed the custom-icons branch 2 times, most recently from 0a2c188 to a54ce02 Compare October 30, 2018 03:36

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Logos can either be a url or a fontawesome name.
Values are set in websites.yml.

Closes manu-chroma#110
@jayvdb jayvdb merged commit f4362ce into manu-chroma:master Oct 30, 2018
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

8 participants