Skip to content

Chrome: Adding a ResponsiveImage component to wrap the featured image #911

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

Merged
youknowriad merged 4 commits into from
Jun 1, 2017
Merged

Chrome: Adding a ResponsiveImage component to wrap the featured image #911

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b3fa3c3
Chrome: Adding a ResponsiveImage component to wrap the featured image…
youknowriad May 26, 2017
50bde7e
Chrome: Featured media, Ignore aborted requests
youknowriad May 26, 2017
2d054a6
Chrome: Transform ResponsiveImage to a generic ResponsiveComponent
youknowriad May 29, 2017
406eb1c
Components: Using Children.count instead of testing the children shape
youknowriad May 31, 2017
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
Next Next commit
Chrome: Adding a ResponsiveImage component to wrap the featured image… 
… props @aduth
  • Loading branch information
@youknowriad
youknowriad committed May 31, 2017
commit b3fa3c34adeb6b8c3dba9aa2e5e196ec8e062932
1 change: 1 addition & 0 deletions components/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export { default as Panel } from './panel';
export { default as PanelHeader } from './panel/header';
export { default as PanelBody } from './panel/body';
export { default as Placeholder } from './placeholder';
export { default as ResponsiveImage } from './responsive-image';
export { default as Spinner } from './spinner';
export { default as Toolbar } from './toolbar';

Expand Down
18 changes: 18 additions & 0 deletions components/responsive-image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Internal dependencies
*/
import './style.scss';

function ResponsiveImage( { src, alt, naturalWidth, naturalHeight } ) {
const imageStyle = {
paddingBottom: ( naturalHeight / naturalWidth * 100 ) + '%',
};
return (
<div className="components-responsive-image">
<div style={ imageStyle } />
<img src={ src } className="components-responsive-image__image" alt={ alt } />
Copy link
Member

@aduth aduth May 26, 2017

Choose a reason for hiding this comment

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

In the future, or perhaps even now, we may want to make this component a bit more generic as a responsive panel. The padding technique is rarely used with images, except in this case where we don't know the rendered width, and often more for background images or video elements. It could also help avoid the need to overwhelm the props with individual img attributes we want to assign, if we instead accepted the img as a child.

Not too concerned about this being addressed here.

Copy link
Contributor Author

@youknowriad youknowriad May 29, 2017

Choose a reason for hiding this comment

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

I tried to handle this, I've renamed ResponsiveImage as ResponsiveWrapper and it works by checking that the children is a single React Element and then cloning the element to add the class name. Does this sound good for you?

</div>
);
}

export default ResponsiveImage;
14 changes: 14 additions & 0 deletions components/responsive-image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
.components-responsive-image {
position: relative;
max-width: 100%;
}

.components-responsive-image__image {
position: absolute;
top: 0;
right: 0;
bottom: 0;
left: 0;
width: 100%;
height: 100%;
}
21 changes: 9 additions & 12 deletions editor/sidebar/featured-image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { connect } from 'react-redux';
*/
import { Component } from 'element';
import { __ } from 'i18n';
import { Button, PanelBody, Spinner } from 'components';
import { Button, PanelBody, Spinner, ResponsiveImage } from 'components';
import { MediaUploadButton } from 'blocks';

/**
Expand Down Expand Up @@ -50,21 +50,17 @@ class FeaturedImage extends Component {
return;
}
this.setState( { loading: true } );
const mediaIdToLoad = this.props.featuredImageId;
this.fetchMediaRequest = new wp.api.models.Media( { id: mediaIdToLoad } ).fetch()
if ( this.fetchMediaRequest ) {
this.fetchMediaRequest.abort();
}
this.fetchMediaRequest = new wp.api.models.Media( { id: this.props.featuredImageId } ).fetch()
.done( ( media ) => {
if ( this.props.featuredImageId !== mediaIdToLoad ) {
return;
}
this.setState( {
loading: false,
media,
} );
} )
.fail( () => {
if ( this.props.featuredImageId !== mediaIdToLoad ) {
Copy link
Member

@aduth aduth May 26, 2017

Choose a reason for hiding this comment

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

Cool 👍 So to be clear, neither the done nor fail case are invoked when the request is aborted?

Copy link
Contributor Author

@youknowriad youknowriad May 26, 2017

Choose a reason for hiding this comment

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

I think so, I tested once but Let me test against to be absolutely certain :)

Copy link
Contributor Author

@youknowriad youknowriad May 26, 2017

Choose a reason for hiding this comment

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

Good catch @aduth the abort was being triggered :( So, now I'm ignoring aborted request in the fail callback.

return;
}
this.setState( {
loading: false,
} );
Expand All @@ -80,15 +76,16 @@ class FeaturedImage extends Component {
<div className="editor-featured-image__content">
{ !! featuredImageId &&
<MediaUploadButton
buttonProps={ { className: 'button-link' } }
buttonProps={ { className: 'button-link editor-featured-image__preview' } }
onSelect={ onUpdateImage }
type="image"
>
{ media &&
<img
className="editor-featured-image__preview"
<ResponsiveImage
src={ media.source_url }
alt={ __( 'Featured image' ) }
naturalWidth={ media.media_details.width }
naturalHeight={ media.media_details.height }
/>
}
{ loading && <Spinner /> }
Expand Down
2 changes: 1 addition & 1 deletion editor/sidebar/featured-image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

.editor-featured-image__preview {
display: block;
max-width: 100%;
width: 100%;
}

.editor-featured-image__howto {
Expand Down