-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix padding in navbar and add gravatar support #70
Conversation
oBlissing
commented
Oct 23, 2017
- Fix navbar having some spacing between menu-items and outer container
- Add a user gravatar if available
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.
Nice work! I love the new look. But let's put this as a method on the User instead of a static function.
xl_auth/utils.py
Outdated
def get_gravatar_url(user_email, size): | ||
"""Get gravatar url from user email.""" | ||
clean_email = str(user_email).lower().encode() | ||
hash = hashlib.md5(clean_email) |
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.
Shadows built-in name hash
. And I think this should be a method on the User
model instead of a stand-alone utility function. That way you can always call current_user.get_gravatar_url(64)
without any fuzz.
Something like this:
def get_gravatar_url(self, size=32):
"""Get Gravatar URL."""
hashed_email = hashlib.md5(str(self.email).lower()).hexdigest()
return 'https://www.gravatar.com/avatar/{}?d=mm&s={}'.format(hashed_email, size)
Should be able to merge this now |
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.
Looking even better! What a neat pull request. Does all it should and nothing else. 👍 Just fix the tiny syntax errors and then we can merge this!
@@ -556,3 +555,6 @@ msgstr "Användarinställningar uppdaterade för \"%(username)s\"." | |||
msgid "Thank you for changing password for \"%(username)s\"." | |||
msgstr "Lösenord för \"%(username)s\" har ändrats." | |||
|
|||
#~ msgid "Logged in as %(user)s" |
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 #~ ...
marks lines that are no longer used and can be removed. So lets remove them!
</a> | ||
</p> | ||
<a class="navbar-link" href="{{ url_for('user.profile') }}"> | ||
<img class="navbar-user-gravatar" src="{{ current_user.get_gravatar_url(32) }}" /> |
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.
"32" is redundant, since you set that as the default in the get_gravatar_url
method. Feel free to keep it though, if you think it adds value. Just mentioning since you're new to Python! 😸
assets/css/style.less
Outdated
} | ||
} | ||
.navbar-user-gravatar { |
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.
@oBlissing Please do the PyCharm + ⌥⌘L and commit again!
Also, I don't think the previous line-40 brace was a "stray" one. The entire .logotype
block is!
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.
Good catch 🥇
@oBlissing I added a simple unittest to your pull request in 94ca74b, hope you don't mind! Merge at will. |