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.

Sign up for GitHub

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

Gravatar: Show current user's local image #9369

Merged
merged 2 commits into from
Nov 17, 2016
Merged

Gravatar: Show current user's local image #9369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84364ed
Show current user's local Gravatar image
v18 Nov 17, 2016
383fa5e
Update to ES6, and clean up syntax
v18 Nov 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/blocks/edit-gravatar/test/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe( 'EditGravatar', function() {
before( function() {
EditGravatar = require( 'blocks/edit-gravatar' ).EditGravatar;
FilePicker = require( 'components/file-picker' );
Gravatar = require( 'components/gravatar' );
Gravatar = require( 'components/gravatar' ).default;
ImageEditor = require( 'blocks/image-editor' );
} );

Expand Down
6 changes: 5 additions & 1 deletion client/components/gravatar/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
Gravatar
======

This component is used to display the [Gravatar](https://gravatar.com/) for a user. It takes a User object as a prop and read the images from user.avatar_URL. The images size is set at 96px, used at smaller sizes for retina display. Using one size allows us to only request one image and cache it on the browser. Even if you are displaying it at smaller sizes you should not change the source image.
This component is used to display the [Gravatar](https://gravatar.com/) for a user. It takes a User object as a prop and read the images from user.avatar_URL.

If the current user has uploaded a new Gravatar recently on Calypso, and therefore has a temporary image set, this component will display the temporary image instead. It reads the temporary image using the redux selector `getUserTempGravatar`.

The images size is set at 96px, used at smaller sizes for retina display. Using one size allows us to only request one image and cache it on the browser. Even if you are displaying it at smaller sizes you should not change the source image.

#### How to use:

Expand Down
101 changes: 67 additions & 34 deletions client/components/gravatar/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,73 +1,106 @@
/**
* External dependencies
*/
import React from 'react';
import React, { Component, PropTypes } from 'react';
import url from 'url';
import qs from 'querystring';
import { connect } from 'react-redux';
import { get } from 'lodash';

/**
* Internal dependencies
*/
import safeImageURL from 'lib/safe-image-url';
import {
getUserTempGravatar
} from 'state/current-user/gravatar-status/selectors';

module.exports = React.createClass( {
displayName: 'Gravatar',
export class Gravatar extends Component {
constructor() {
super( ...arguments );
this.state = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having to write constructor just to set the initial state, I discovered that we can use ES7 static properties to define the initial state as well. This is how:

export class Gravatar extends Component {
   state = {
      failedToLoad: false
   };

 // ... other code follows

Note: I'm not 100% sure it'll work, actually. Haven't tried myself. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean property initializers right, since we don't want this to be static. 😄 I'm fine with the above usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving as-is for now.

failedToLoad: false
};
}

propTypes: {
user: React.PropTypes.object,
size: React.PropTypes.number,
imgSize: React.PropTypes.number
},
static propTypes = {
user: PropTypes.object,
size: PropTypes.number,
imgSize: PropTypes.number,
// connected props:
tempImage: PropTypes.oneOfType( [
PropTypes.string, // the temp image base64 string if it exists
PropTypes.bool // or false if the temp image does not exist
] ),
};

getDefaultProps() {
static defaultProps = {
// The REST-API returns s=96 by default, so that is most likely to be cached
return {
imgSize: 96,
size: 32
};
},

getInitialState() {
return {
failedToLoad: false
};
},
imgSize: 96,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this in px? If yes, could we add it as an inline comment, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm! Not really sure about adding a comment here since it's not obvious at first glance what imgSize and size are there for, and if they both have a px comment it may get more confusing. Both of these are described in the README - is that enough to go on, what do you think @lamosty?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's in README (sorry for not noticing), it's good enough. :)

size: 32
};

getResizedImageURL( imageURL ) {
const { imgSize } = this.props;
imageURL = imageURL || 'https://www.gravatar.com/avatar/0';
const parsedURL = url.parse( imageURL );
const query = qs.parse( parsedURL.query );

if ( /^([-a-zA-Z0-9_]+\.)*(gravatar.com)$/.test( parsedURL.hostname ) ) {
query.s = this.props.imgSize;
query.s = imgSize;
query.d = 'mm';
} else {
// assume photon
query.resize = this.props.imgSize + ',' + this.props.imgSize;
query.resize = imgSize + ',' + imgSize;
}

parsedURL.search = qs.stringify( query );
return url.format( parsedURL );
},
}

onError() {
this.setState( { failedToLoad: true } );
},
onError = () => this.setState( { failedToLoad: true } );

render() {
const size = this.props.size;
const {
alt,
size,
tempImage,
user,
} = this.props;

if ( ! user ) {
return (
<span
className="gravatar is-placeholder"
style={ { width: size, height: size } }
/>
);
}

if ( ! this.props.user ) {
return <span className="gravatar is-placeholder" style={ { width: size, height: size } } />;
} else if ( this.state.failedToLoad ) {
if ( this.state.failedToLoad ) {
return <span className="gravatar is-missing" />;
}

const alt = this.props.alt || this.props.user.display_name;
const avatarURL = this.getResizedImageURL( safeImageURL( this.props.user.avatar_URL ) );
const altText = alt || user.display_name;

const avatarURL = (
tempImage ||
this.getResizedImageURL( safeImageURL( user.avatar_URL ) )
);

return (
<img alt={ alt } className="gravatar" src={ avatarURL } width={ size } height={ size } onError={ this.onError } />
<img
alt={ altText }
className="gravatar"
src={ avatarURL }
width={ size }
height={ size }
onError={ this.onError }
/>
);
}
} );
}

export default connect( ( state, ownProps ) => ( {
tempImage: getUserTempGravatar( state, get( ownProps, 'user.ID', false ) ),
} ) )( Gravatar );
60 changes: 40 additions & 20 deletions client/components/gravatar/test/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@

/**
* External dependencies
*/
var assert = require( 'assert' ),
ReactDomServer = require( 'react-dom/server' ),
React = require( 'react' );
import assert from 'assert';
import ReactDomServer from 'react-dom/server';
import React from 'react';

/**
* Internal dependencies
*/
var Gravatar = require( '../' );
import { Gravatar } from '../';

/**
* Pass in a react-generated html string to remove react-specific attributes
Expand All @@ -22,53 +21,74 @@ function stripReactAttributes( string ) {
}

describe( 'Gravatar', function() {
var bobTester = {
const bobTester = {
avatar_URL: 'https://0.gravatar.com/avatar/cf55adb1a5146c0a11a808bce7842f7b?s=96&d=identicon',
display_name: 'Bob The Tester'
};

describe( 'rendering', function() {
it( 'should render an image given a user with valid avatar_URL, with default width and height 32', function() {
var gravatar = <Gravatar user={ bobTester } />,
expectedResultString = '<img alt="Bob The Tester" class="gravatar" src="https://0.gravatar.com/avatar/cf55adb1a5146c0a11a808bce7842f7b?s=96&amp;d=mm" width="32" height="32"/>';
const gravatar = <Gravatar user={ bobTester } />;
const expectedResultString = '<img alt="Bob The Tester" ' +
'class="gravatar" ' +
'src="https://0.gravatar.com/' +
'avatar/cf55adb1a5146c0a11a808bce7842f7b?s=96&amp;d=mm" ' +
'width="32" height="32"/>';

assert.equal( expectedResultString, stripReactAttributes( ReactDomServer.renderToStaticMarkup( gravatar ) ) );
Copy link
Member

Choose a reason for hiding this comment

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

@v18 I highly recommend you check out enzyme() for testing. it can make testing React components significantly easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmsnell - these tests were written before my time. I'm not sure that updating the tests for this PR falls under the scope here :) The tests I wrote for a different PR use enzyme <3

Copy link
Member

Choose a reason for hiding this comment

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

@v18 as you will. I would hate to add more cruft if it's not too much more work to update there. wouldn't have to change the whole file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmsnell I know what you mean :) On the other hand, to change the tests, I'd need to rework the entire file, no? They all rely on checking the final HTML output vs checking properties.

} );

it( 'should update the width and height when given a size attribute', function() {
var gravatar = <Gravatar user={ bobTester } size={ 100 } />,
expectedResultString = '<img alt="Bob The Tester" class="gravatar" src="https://0.gravatar.com/avatar/cf55adb1a5146c0a11a808bce7842f7b?s=96&amp;d=mm" width="100" height="100"/>';
const gravatar = <Gravatar user={ bobTester } size={ 100 } />;
const expectedResultString = '<img alt="Bob The Tester" ' +
'class="gravatar" ' +
'src="https://0.gravatar.com/' +
'avatar/cf55adb1a5146c0a11a808bce7842f7b?s=96&amp;d=mm" ' +
'width="100" height="100"/>';

assert.equal( expectedResultString, stripReactAttributes( ReactDomServer.renderToStaticMarkup( gravatar ) ) );
} );

it( 'should update source image when given imgSize attribute', function() {
var gravatar = <Gravatar user={ bobTester } imgSize={ 200 } />,
expectedResultString = '<img alt="Bob The Tester" class="gravatar" src="https://0.gravatar.com/avatar/cf55adb1a5146c0a11a808bce7842f7b?s=200&amp;d=mm" width="32" height="32"/>';
const gravatar = <Gravatar user={ bobTester } imgSize={ 200 } />;
const expectedResultString = '<img alt="Bob The Tester" ' +
'class="gravatar" ' +
'src="https://0.gravatar.com/' +
'avatar/cf55adb1a5146c0a11a808bce7842f7b?s=200&amp;d=mm" ' +
'width="32" height="32"/>';

assert.equal( expectedResultString, stripReactAttributes( ReactDomServer.renderToStaticMarkup( gravatar ) ) );
} );

it( 'should serve a default image if no avatar_URL available', function() {
var noImageTester = { display_name: 'Bob The Tester' },
gravatar = <Gravatar user={ noImageTester } />,
expectedResultString = '<img alt="Bob The Tester" class="gravatar" src="https://www.gravatar.com/avatar/0?s=96&amp;d=mm" width="32" height="32"/>';
const noImageTester = { display_name: 'Bob The Tester' };
const gravatar = <Gravatar user={ noImageTester } />;
const expectedResultString = '<img alt="Bob The Tester" ' +
'class="gravatar" ' +
'src="https://www.gravatar.com/avatar/0?s=96&amp;d=mm" ' +
'width="32" height="32"/>';

assert.equal( expectedResultString, stripReactAttributes( ReactDomServer.renderToStaticMarkup( gravatar ) ) );
} );

it( 'should allow overriding the alt attribute', function() {
var gravatar = <Gravatar user={ bobTester } alt="Alternate Alt" />,
expectedResultString = '<img alt="Alternate Alt" class="gravatar" src="https://0.gravatar.com/avatar/cf55adb1a5146c0a11a808bce7842f7b?s=96&amp;d=mm" width="32" height="32"/>';
const gravatar = <Gravatar user={ bobTester } alt="Alternate Alt" />;
const expectedResultString = '<img alt="Alternate Alt" ' +
'class="gravatar" ' +
'src="https://0.gravatar.com/' +
'avatar/cf55adb1a5146c0a11a808bce7842f7b?s=96&amp;d=mm" ' +
'width="32" height="32"/>';

assert.equal( expectedResultString, stripReactAttributes( ReactDomServer.renderToStaticMarkup( gravatar ) ) );
} );

// I believe jetpack sites could have custom avatars, so can't assume it's always a gravatar
it( 'should promote non-secure avatar urls to secure', function() {
var nonSecureTester = { avatar_URL: 'http://www.example.com/avatar' },
gravatar = <Gravatar user={ nonSecureTester } />,
expectedResultString = '<img class="gravatar" src="https://i2.wp.com/www.example.com/avatar?resize=96%2C96" width="32" height="32"/>';
const nonSecureTester = { avatar_URL: 'http://www.example.com/avatar' };
const gravatar = <Gravatar user={ nonSecureTester } />;
const expectedResultString = '<img class="gravatar" ' +
'src="https://i2.wp.com/www.example.com/avatar?resize=96%2C96" ' +
'width="32" height="32"/>';

assert.equal( expectedResultString, stripReactAttributes( ReactDomServer.renderToStaticMarkup( gravatar ) ) );
} );
Expand Down
4 changes: 2 additions & 2 deletions client/components/user/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
/**
* External dependencies
*/
var React = require( 'react' );
import React from 'react';

/**
* Internal dependencies
*/
var Gravatar = require( 'components/gravatar' );
import Gravatar from 'components/gravatar';

module.exports = React.createClass( {
displayName: 'UserItem',
Expand Down
2 changes: 1 addition & 1 deletion client/my-sites/people/delete-user/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ const Card = require( 'components/card' ),
FormButtonsBar = require( 'components/forms/form-buttons-bar' ),
AuthorSelector = require( 'blocks/author-selector' ),
UsersActions = require( 'lib/users/actions' ),
Gravatar = require( 'components/gravatar' ),
accept = require( 'lib/accept' ),
analytics = require( 'lib/analytics' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a blank line below so old requires and new imports are separated. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it out with a new line, and the extra space makes it seem like <Gravatar /> is not an internal dependency, so I guess it's a bit strange either way. It looks ok to me without the extra line since the import keyword is in a new block-ish, so thinking of leaving as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should just follow up by porting the rest of the requires to imports in a janitorial PR. Spacing isn't a huge deal for now.

import Gravatar from 'components/gravatar';

module.exports = React.createClass( {
displayName: 'DeleteUser',
Expand Down
22 changes: 14 additions & 8 deletions client/post-editor/editor-author/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
/**
* External dependencies
*/
const React = require( 'react' );
import React from 'react';

/**
* Internal dependencies
*/
const Gravatar = require( 'components/gravatar' ),
user = require( 'lib/user' )(),
AuthorSelector = require( 'blocks/author-selector' ),
PostActions = require( 'lib/posts/actions' ),
touchDetect = require( 'lib/touch-detect' ),
sites = require( 'lib/sites-list' )(),
stats = require( 'lib/posts/stats' );
import Gravatar from 'components/gravatar';
import userFactory from 'lib/user';
import AuthorSelector from 'blocks/author-selector';
import PostActions from 'lib/posts/actions';
import touchDetect from 'lib/touch-detect';
import sitesFactory from 'lib/sites-list';
import * as stats from 'lib/posts/stats';

/**
* Module dependencies
*/
const user = userFactory();
const sites = sitesFactory();

export default React.createClass( {
displayName: 'EditorAuthor',
Expand Down
3 changes: 2 additions & 1 deletion client/state/current-user/gravatar-status/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ describe( 'selectors', () => {
const currentUserId = 1;
const anotherUserId = 2;

it( 'returns false if user ID is not passed in', () => {
it( 'returns false if user ID is not passed in, or is false', () => {
const state = {
currentUser: {
gravatarStatus: {
Expand All @@ -57,6 +57,7 @@ describe( 'selectors', () => {
}
};
expect( getUserTempGravatar( state ) ).to.equal( false );
expect( getUserTempGravatar( state, false ) ).to.equal( false );
} );

it( 'returns false if the user ID passed is not the current user ID', () => {
Expand Down