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

OFFLINE: Gravatar fallback for offline #2858

Merged
merged 1 commit into from
Jan 30, 2016
Merged

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Jan 27, 2016

We always load gravatars from CDN and they are not cached for some reason
Also, gravatar does not support CORS so any caching attempts with serviceWorkers have failed.

This is the most straightforward approach to add a gravatar fallback for both APP and WEB.

I'm just hooking into onError event.

@rralian @mtias @johngodley @gziolo

@artpi artpi added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 27, 2016
let avatarURL = this._getResizedImageURL( safeImageURL( this.props.user.avatar_URL ) );

if ( this.state.failedToLoad ) {
avatarURL = 'calypso/images/me/gravatar.png';
Copy link
Contributor

Choose a reason for hiding this comment

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

need a leading slash here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rralian
Copy link
Contributor

rralian commented Jan 27, 2016

So, this is kinda funny, but the intent is to provide a fallback while offline, right? Except... that fallback image is not available offline... at least not in the browser (I believe it would be in the desktop editor). Could we instead provide the fallback via data URI?

offline bammage

@artpi
Copy link
Contributor Author

artpi commented Jan 28, 2016

that fallback image is not available offline

I was mostly targeting app and broken whole gravatar infrastructure.
I will do that, although if '/calypso' dir does not work we have bigger problems to worry about

@mtias mtias added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jan 28, 2016
@mtias
Copy link
Member

mtias commented Jan 28, 2016

@mattmiklic should we take this chance to revisit the default avatar we show in Calypso if we are serving an image ourselves? At least maybe we could adjust the neutral grey?

@mattmiklic
Copy link
Member

Yes! I just put together a new default gravatar image for the iOS app, using the user Gridicon. I'll export a version for Calypso.

@mattmiklic
Copy link
Member

Here's a direct export in SVG format. If it needs to be resized or in a different format or anything, let me know. (Zipped because GitHub doesn't allow SVGs in comments)

default-gravatar.svg.zip

@folletto
Copy link
Contributor

I'd note that offline we can just drop the avatar and leave just the icon, since that's already a normal behaviour on native apps (showing only the icon). I feel would be a better choice for the masterbar. :)

In other instances of the gravatar tho, we can use a custom offline gravatar alternative. :)

In terms of masterbar specific behaviour it should show the Me icon until an images is loaded, and show it only once it gets loaded. :)

@artpi
Copy link
Contributor Author

artpi commented Jan 28, 2016

Currently we are showing blinking placeholder unil icon is loaded.
Thanks @mattmiklic, its SVG so I guess we can throw it inline, that will solve @rralian 's concerns beautifully!

@mattmiklic
Copy link
Member

I'd note that offline we can just drop the avatar and leave just the icon, since that's already a normal behaviour on native apps (showing only the icon). I feel would be a better choice for the masterbar. :)

Definitely, if the gravatar in the Me link in the masterbar fell back to gridicons-user-circle when the user doesn't have a gravatar, that'd be the best-looking option.

@artpi
Copy link
Contributor Author

artpi commented Jan 28, 2016

Definitely, if the gravatar in the Me link in the masterbar fell back to gridicons-user-circle when the user doesn't have a gravatar, that'd be the best-looking option.

@mtias , @adambbecker , do you agree? You were unhappy with the design of this icon

@adambbecker
Copy link
Contributor

@artpi: 👍

@artpi
Copy link
Contributor Author

artpi commented Jan 28, 2016

Now offline gravatar looks like this:

I think this is the cleanest approach, addresses @rralian , @mattmiklic , @folletto 's and @mtias 's concerns ( I hope )

@folletto
Copy link
Contributor

For me that's ideal. 👍

@mattmiklic
Copy link
Member

Now offline gravatar looks like this:

Is that gridicons-user-circle? The outside border looks too thick compared to the standard gridicon to me (mocked up for comparison):

screen shot 2016-01-28 at 11 26 27 am

@artpi
Copy link
Contributor Author

artpi commented Jan 28, 2016

I actually didnt touch it :) It was introduced somewhere else
The border looks thick but I would vote for changing it in a different PR

It was there before:

@mattmiklic
Copy link
Member

Is it because a border is being drawn around the gravatar image in CSS, maybe?

@mtias
Copy link
Member

mtias commented Jan 28, 2016

You were unhappy with the design of this icon

@artpi I don't remember this :)

Is it because a border is being drawn around the gravatar image in CSS, maybe?

Indeed. We should remove it.

@artpi
Copy link
Contributor Author

artpi commented Jan 28, 2016

@artpi I don't remember this :)

My bad :)

@mtias
Copy link
Member

mtias commented Jan 29, 2016

👍

OFFLINE: Gravatar fallback for offline

OFFLINE: gravatar fallback: add leading slash

OFFLINE gravatar: ternary fallback

OFFLINE gravatar fallback: change to default icon
@artpi artpi force-pushed the update/gravatar-fallback branch from 72bea9a to 6b1f1a3 Compare January 29, 2016 21:43
@rralian rralian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 29, 2016
artpi added a commit that referenced this pull request Jan 30, 2016
@artpi artpi merged commit f35273d into master Jan 30, 2016
@artpi artpi deleted the update/gravatar-fallback branch January 30, 2016 12:10
@lancewillett lancewillett removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants