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

adding in user icon in the desktop notifications #730

Closed
wants to merge 1 commit into from
Closed

adding in user icon in the desktop notifications #730

wants to merge 1 commit into from

Conversation

soonahn
Copy link
Contributor

@soonahn soonahn commented Sep 7, 2015

Address #729

@rodrigok
Copy link
Member

rodrigok commented Sep 8, 2015

@soonahn awesome, this works synchronously?

@rodrigok
Copy link
Member

rodrigok commented Sep 8, 2015

Seems that it isn't, if avatar (initials) not was loaded before the notification the notification has no image.

@soonahn
Copy link
Contributor Author

soonahn commented Sep 8, 2015

Synconous on the client side. Feature of html5 and canvas to transform image to png. Does require building a canvas element, but doesn't attach to the DOM

@soonahn
Copy link
Contributor Author

soonahn commented Sep 8, 2015

Well yes, it's using the same method to get the avatar as everything else is. Is this an issue?

@rodrigok
Copy link
Member

rodrigok commented Sep 8, 2015

No, you didn't understand, if the image isn't in browser cache you are rendering an empty image because render is sync but the code doesn't stop while the image is loaded from server.

@rodrigok
Copy link
Member

rodrigok commented Sep 8, 2015

Steps to reproduce:

  • Login with user A and user B
  • With user A got to some room where you can't see user B, like a private room with another user and do a refresh clearing the browser cache
  • With user B send a direct message to user A
  • User A will receive a notification from user B without the avatar of user B (if it is initials)

@soonahn
Copy link
Contributor Author

soonahn commented Sep 8, 2015

Ahh, now I understand. What is the right solution? Grab the user's initials avatar, load it into the browser cache, then send the notification? Seems weird to delay the notification, I would rather have a notification without an image, than a notification that came at the wrong time.

@geekgonecrazy
Copy link
Contributor

What about something like:

img.onload = ->
  //proceed cause image should have now loaded

Not much of a delay.. also I haven't tested this. But i'd think that'd insure the image was there before running through the canvas.

Especially since avatarUrlFromUsername just returns a url:

return "#{Meteor.absoluteUrl()}avatar/#{username}.jpg?_dc=#{random}"

@rodrigok
Copy link
Member

rodrigok commented Sep 8, 2015

@geekgonecrazy good ideia 😄

@soonahn
Copy link
Contributor Author

soonahn commented Sep 8, 2015

@geekgonecrazy good idea, I'll look at it again tonight. Thanks guys!

@geekgonecrazy
Copy link
Contributor

@soonahn let me know if you need help with this.

@soonahn
Copy link
Contributor Author

soonahn commented Sep 9, 2015

Am just busy with work. Weekdays are hard for me, I'll get it done this weekend, unless someone wants to take this branch over.

@engelgabriel
Copy link
Member

It's fine @soonahn

we appreciate all the help we can get. :)

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.

4 participants