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

Addition of favicon and coala logo #16

Merged
merged 1 commit into from
Dec 8, 2017
Merged

Conversation

dob9601
Copy link
Contributor

@dob9601 dob9601 commented Dec 3, 2017

closes #1

gci/views.py Outdated
@@ -14,6 +14,7 @@ def index(request):
org_id = linked_students[0]['organization_id']
org_name = linked_students[0]['organization_name']
s = []
s.append('<image href="images/coala_logo.png">')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename this image to org_logo.png? That'll fix the Travis build.

@dob9601
Copy link
Contributor Author

dob9601 commented Dec 3, 2017

@andrewda I’ll give it a go now - was strugging to make sense of the build log

@dob9601 dob9601 force-pushed the favicon branch 3 times, most recently from e7a2cf0 to 1b8b582 Compare December 3, 2017 23:38
@jayvdb
Copy link
Member

jayvdb commented Dec 4, 2017

The solution must work for any org. Per #1, "detect the name of the GitHub org from the Travis environment variables, and fetch the org's logo."

@dob9601
Copy link
Contributor Author

dob9601 commented Dec 4, 2017

@jayvdb sorry, should have read the issue in full before starting. Where would you find this environment variable? (couldn’t see any variables in .travis.yml relating to the org using the project)

@dob9601
Copy link
Contributor Author

dob9601 commented Dec 4, 2017

never mind, I think I found it in GitOrg.py

@dob9601 dob9601 force-pushed the favicon branch 2 times, most recently from 0e2a3e9 to 0548211 Compare December 4, 2017 23:31
gci/logo.py Outdated
image_url_short = "http://github.com/%s.png" % (org_name)

#Follow the redirect to the page containing the image
response = urllib.request.urlopen(IMAGE_URL_SHORT)
Copy link
Member

Choose a reason for hiding this comment

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

please use pypi package requests

gci/logo.py Outdated
import urllib.request

def get_logo():
#Obtain the org name from the Travis environment variables
Copy link
Member

Choose a reason for hiding this comment

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

check your code with pyflakes & pycodestyle

gci/views.py Outdated
get_logo()

s.append('<link rel="shortcut icon" type="image/png" href="images/favicon.png"/>')
s.append('<image href="images/favicon.png">')
Copy link
Member

Choose a reason for hiding this comment

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

What's this for? Seems like it's not needed

Copy link
Member

Choose a reason for hiding this comment

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

@dob9601 Line 22

Copy link
Member

Choose a reason for hiding this comment

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

A review is usually given to the bottommost line in the "review window".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my mistake. Line 22 displays the image. Line 21 is a link to the favicon since it is in a png format and in a subfolder

Copy link
Member

Choose a reason for hiding this comment

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

Did you mean <img> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can’t believe I did that :( must have been half asleep when working on that

@dob9601
Copy link
Contributor Author

dob9601 commented Dec 5, 2017

@blazeu could you narrow down to one line specifically

@dob9601
Copy link
Contributor Author

dob9601 commented Dec 5, 2017

@jayvdb Do you want me to approach the favicon with full backward compatibility (the creation of a 32x32 .ico file instead of just linking to a PNG)?

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 7.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -4,7 +4,7 @@
 
 def get_logo():
     # Obtain the org name from the Travis environment variables
-    org_name = os.environ["TRAVIS_REPO_SLUG"].split("/")[0]
+    org_name = os.environ['TRAVIS_REPO_SLUG'].split("/")[0]
 
     image_url_short_max_res = "http://github.com/%s.png" % (org_name)
 

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 7.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -4,7 +4,7 @@
 
 def get_logo():
     # Obtain the org name from the Travis environment variables
-    org_name = os.environ["TRAVIS_REPO_SLUG"].split("/")[0]
+    org_name = os.environ["TRAVIS_REPO_SLUG"].split('/')[0]
 
     image_url_short_max_res = "http://github.com/%s.png" % (org_name)
 

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 9.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -6,7 +6,7 @@
     # Obtain the org name from the Travis environment variables
     org_name = os.environ["TRAVIS_REPO_SLUG"].split("/")[0]
 
-    image_url_short_max_res = "http://github.com/%s.png" % (org_name)
+    image_url_short_max_res = 'http://github.com/%s.png' % (org_name)
 
     # Follow the redirect to the page containing the image and
     # store it in the response variable

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 16.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -13,7 +13,7 @@
     response_max_res = requests.get(image_url_short_max_res)
 
     # Write the image to a file and save
-    image = open("images/org_logo.png", "wb")
+    image = open('images/org_logo.png', "wb")
     image.write(response_max_res.content)
     image.close()
 

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 16.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -13,7 +13,7 @@
     response_max_res = requests.get(image_url_short_max_res)
 
     # Write the image to a file and save
-    image = open("images/org_logo.png", "wb")
+    image = open("images/org_logo.png", 'wb')
     image.write(response_max_res.content)
     image.close()
 

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 21.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -18,7 +18,7 @@
     image.close()
 
     # Run the same code again but download a 16x16 version for favicon
-    image_url_short_favicon = "http://github.com/%s.png?size=16" % (org_name)
+    image_url_short_favicon = 'http://github.com/%s.png?size=16' % (org_name)
 
     response_favicon = requests.get(image_url_short_favicon)
 

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 25.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -22,6 +22,6 @@
 
     response_favicon = requests.get(image_url_short_favicon)
 
-    image = open("favicon.png", "wb")
+    image = open('favicon.png', "wb")
     image.write(response_favicon.content)
     image.close()

@jayvdb
Copy link
Member

jayvdb commented Dec 5, 2017

Comment on 024634d, file gci/logo.py, line 25.

You do not use the preferred quotation marks.

Origin: QuotesBear, Section: all.python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmppppvnesy/gci/logo.py
+++ b/tmp/tmppppvnesy/gci/logo.py
@@ -22,6 +22,6 @@
 
     response_favicon = requests.get(image_url_short_favicon)
 
-    image = open("favicon.png", "wb")
+    image = open("favicon.png", 'wb')
     image.write(response_favicon.content)
     image.close()

gci/logo.py Outdated

response_favicon = requests.get(image_url_short_favicon)

image = open('favicon.png', 'wb')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it rather be favicon.ico?

more on this here

Copy link
Contributor Author

@dob9601 dob9601 Dec 5, 2017

Choose a reason for hiding this comment

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

@nkprince007 most modern browsers support pngs
https://en.wikipedia.org/wiki/Favicon#File_format_support
(every one on wikipedia listed does)

gci/gitorg.py Outdated
else:
image_url = 'http://github.com/%s.png' % (org_name)

return requests.get(image_url_short).content
Copy link
Member

Choose a reason for hiding this comment

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

not tested :P

@jayvdb
Copy link
Member

jayvdb commented Dec 7, 2017

yay; build errors fixed. here is what it looks like : https://pr-16--wonderful-williams-adc3fd.netlify.com/

@dob9601 dob9601 force-pushed the favicon branch 2 times, most recently from 5034214 to 5df8df0 Compare December 7, 2017 16:34
gci/views.py Outdated
org_logo_file.write(org_logo)

s.append('<link rel="shortcut icon" type="image/png" href="favicon.png"/>')
s.append('<img src="org_logo.png" alt='+org_name+'>')
Copy link
Member

Choose a reason for hiding this comment

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

alt="'+org_name+'"

gci/views.py Outdated
with open('static/org_logo.png', 'wb') as org_logo_file:
org_logo_file.write(org_logo)

s.append('<link rel="shortcut icon" type="image/png" href="favicon.png"/>')
Copy link
Member

@jayvdb jayvdb Dec 7, 2017

Choose a reason for hiding this comment

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

you need to add static/ to these URLs

gci/views.py Outdated
@@ -16,6 +17,17 @@ def index(request):
org_id = linked_students[0]['organization_id']
org_name = linked_students[0]['organization_name']
s = []

favicon = get_logo(org_name, 16)
with open('static/favicon.png', 'wb') as favicon_file:
Copy link
Member

Choose a reason for hiding this comment

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

And this needs to be _site/...

gci/views.py Outdated

favicon = get_logo(org_name, 16)
with open('_site/favicon.png', 'wb') as favicon_file:
favicon_file.write(favicon)
Copy link
Collaborator

Choose a reason for hiding this comment

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

E501 line too long (86 > 80 characters)

Origin: PycodestyleBear (E501), Section: all.autopep8.

gci/views.py Outdated

favicon = get_logo(org_name, 16)
with open('_site/favicon.png', 'wb') as favicon_file:
favicon_file.write(favicon)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is longer than allowed. (86 > 80)

Origin: LineLengthBear, Section: all.linelength.

Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

you have added the submodule "out"

@jayvdb
Copy link
Member

jayvdb commented Dec 8, 2017

unack 4ee9f38

@jayvdb
Copy link
Member

jayvdb commented Dec 8, 2017

Please squash all the commits into one commit.

@jayvdb
Copy link
Member

jayvdb commented Dec 8, 2017

Needs another rebase, and please squash them into one commit.


Conflicting files
gci/views.py

@jayvdb
Copy link
Member

jayvdb commented Dec 8, 2017

unack 5ebcf0f

@jayvdb
Copy link
Member

jayvdb commented Dec 8, 2017

uglyaf https://pr-16--wonderful-williams-adc3fd.netlify.com/ , but it works.

@jayvdb
Copy link
Member

jayvdb commented Dec 8, 2017

ack eed7bd9

@blazeu
Copy link
Member

blazeu commented Dec 8, 2017

@Mixih
Copy link
Member

Mixih commented Dec 8, 2017

ack eed7bd9

@jayvdb
Copy link
Member

jayvdb commented Dec 8, 2017

pr-16--wonderful-williams-adc3fd.netlify.com/static/timeago.js
^ 404 ?

Netlify build isnt the same as the Travis build

@jayvdb jayvdb merged commit eed7bd9 into coala:master Dec 8, 2017
@dob9601 dob9601 deleted the favicon branch December 8, 2017 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add org logo and a favicon