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

use initial avatars instead of 'mystery person' when gravatar not set #3043

Merged
merged 1 commit into from
Apr 15, 2016

Conversation

macqueen
Copy link
Contributor

fixes #2856

screenshot 2016-04-12 15 52 13

@getsentry/ui @mattrobenolt @dcramer

@codecov-io
Copy link

Current coverage is 83.11%

Merging #3043 into master will decrease coverage by -0.02% as of bc44f61

@@            master   #3043   diff @@
======================================
  Files          924     926     +2
  Stmts        36202   36265    +63
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit          30097   30141    +44
  Partial          0       0       
- Missed        6105    6124    +19

Review entire Coverage Diff as of bc44f61

Powered by Codecov. Updated on successful CI builds.

@@ -374,13 +375,13 @@ def _handle_unknown_identity(self, identity):
if not op:
if request.user.is_authenticated():
return self.respond('sentry/auth-confirm-link.html', {
'identity': identity,
'identity': InterfaceUser.to_python(identity),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not totally sure this is a good idea, so let me know if there are better ways to do this

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you're trying to do here. This is a little odd, but we can talk about it in person since it's not 100% clear to me what made you do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically, this was a way to use the User interface's get_label and get_display_name methods, which makes passing different user-like objects into the templating function a lot easier.. but yes, should probably talk in person.

@ckj
Copy link
Member

ckj commented Apr 12, 2016

letars

So good. Pumped to have this live.



def hash_user_identifier(identifier):
return sum(map(ord, identifier))
Copy link
Member

Choose a reason for hiding this comment

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

not sure how much we care about this, but pretty sure this % 6 will not be very uniform.

Copy link
Member

Choose a reason for hiding this comment

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

The code in the colorhash module or what it's called truncates an md5 which distributes better but is also slower.

Copy link
Member

Choose a reason for hiding this comment

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

Oh i see. We also use this in JS. Not worth the effort then.

@dcramer
Copy link
Member

dcramer commented Apr 12, 2016

hero

return (
u'<svg viewBox="0 0 120 120" xmlns="http://www.w3.org/2000/svg" {size_attrs}>'
'<rect x="0" y="0" width="120" height="120" rx="15" ry="15" fill={color}></rect>'
'<text x="50%" y="50%" font-size="65" dominant-baseline="central" text-anchor="middle" fill="#FFFFFF">'
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this different from what you're doing in JSX? In JSX you're doing style={{'dominantBaseline': 'central'}}, which doesn't translate to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the correct way to do this, but the version of react that we're using doesn't support the dominant-baseline svg property (it should soon, see facebook/react#5714). adding it as a style property has the same effect, so just doing that for now. i should maybe add a TODO to update the jsx version whenever we upgrade react though

@macqueen macqueen force-pushed the letars branch 2 times, most recently from 08cbc20 to 1cf22cf Compare April 15, 2016 01:41
def to_email_html(self, event, **kwargs):
context = {
'user_id': self.id,
'user_email': self.email,
'user_username': self.username,
'user_ip_address': self.ip_address,
'user_data': self.data,
'user': self
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing commas in python plz. :)

@mattrobenolt
Copy link
Contributor

I didn't test the email rendering, but everything else seems reasonable aside from the minor adjustments.

@macqueen macqueen merged commit d9953f5 into master Apr 15, 2016
@macqueen macqueen deleted the letars branch April 15, 2016 18:14
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default to 'identicon' instead of 'mystery man' for gravatar
7 participants