-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are quotes necessary here?
There was a problem hiding this comment.
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.
34c26dd
to
42ec582
Compare
websites.yml
Outdated
@@ -144,3 +144,6 @@ avatar: | |||
tumblr: opengraph | |||
twitter: | |||
url: https://twitter.com/{u}/profile_image?size=original | |||
icon_css: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
templates/status.html
Outdated
|
||
function determineHtmlElement(htmlAttribute) { | ||
var elementName; | ||
if(htmlAttribute == 'src') { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 (
There was a problem hiding this comment.
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?
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. |
7066fff
to
01df69a
Compare
There was a problem hiding this 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
templates/status.html
Outdated
return element; | ||
} | ||
|
||
function determineHtmlAttribute(key) { |
There was a problem hiding this comment.
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.
templates/status.html
Outdated
return logoElement; | ||
} | ||
|
||
function determineHtmlElement(htmlAttribute) { |
There was a problem hiding this comment.
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.
0a2c188
to
a54ce02
Compare
Logos can either be a url or a fontawesome name. Values are set in websites.yml. Closes manu-chroma#110
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 specifyingsrc : ...
it will construct an image tag, and by specifyingclass: ...
it will construct ani
tag.This code desperately needs a second pair of eyes to look at it and suggest feedback!