Chrome: Adding the featured image panel #894
Conversation
youknowriad
added
the
General Interface
youknowriad
force-pushed
the
add/featured-image
branch
from
b2149a2
to
b19a55e
Compare
|
I love it, works well, exactly how it should be. Zero concerns from my side. 👍 👍 |
| @@ -0,0 +1,3 @@ | |||
| const spinner = <span className="spinner is-active" />; | |||
There was a problem hiding this comment.
I have a good teacher :)
| loading: true, | ||
| } ); | ||
| const mediaIdToLoad = this.props.featuredImageId; | ||
| new wp.api.models.Media( { id: mediaIdToLoad } ).fetch() |
There was a problem hiding this comment.
Backbone.Model#fetch returns a jqXHR (reference), which itself is a superset of XMLHttpRequest (reference), which makes available an abort method (reference).
Maybe rather than tracking this.mounted, we can store a reference to the request here and abort it if the component unmounts? I'm not sure what effect this has on promise resolution though.
There was a problem hiding this comment.
Works well in my testings
| fetchMedia() { | ||
| if ( ! this.props.featuredImageId ) { | ||
| this.setState( { | ||
| media: null, |
There was a problem hiding this comment.
Maybe moving up the common media: null would make it more reasonable to turn setters into one-liners to trim a few lines and clarify the common behavior.
this.setState( { media: null } );
if ( ! this.props.featuredImageId ) {
this.setState( { loading: false } );
return;
}
this.setState( { loading: true } );It's unclear from the documentation whether the current implementation would cause the render to occur twice by separate calls to setState, so maybe worth avoiding if it does.
There was a problem hiding this comment.
I tried it and it doesn't render twice. updated
|
Planning to merge this soon. Let me know if there's any concern. |
| type="image" | ||
| > | ||
| { media && | ||
| <img |
There was a problem hiding this comment.
One minor thing we might consider is using the media's dimensions to assign a height to the space that the image will occupy, to avoid the jarring appearance of content being pushed down only after the image has loaded.
Height is difficult to calculate unless we know the width of the container, which we probably don't want to assume. We could use a technique for proportionally scaled responsive containers though, using padding to dynamically fill height.
See:
There was a problem hiding this comment.
I'm not sure I understand. The height depends on the loaded image. So even if we know the width container, we don't know the ratio of the loaded image, how can we compute the height in this case?
There was a problem hiding this comment.
To your point, more accurately there's two resources that need to be loaded:
- The media entity REST API resource
- The image itself
The idea is that after the first of these finishes loading, we know the dimensions of the image and the space we expect it to consume, even before the image itself has finished loading.
Practically speaking this is probably not very noticeable though, since either we've loaded a post with a featured image already assigned and Settings -> Featured Image is likely collapsed, or we've just assigned the image from the media library and the image is already cached by the browser.
There was a problem hiding this comment.
Here's maybe a clearer demonstration of the issue/solution and its particular relevance when we don't know the containing width:
closes #855
This PR adds the featured image panel. Its behaviour is similar to the current WordPress editor featured image panel.
This raises the question of async loading of WP entities (media). Should we store those in the state? Should we provide QueryComponents (ala Calypso). For now, to limit the scope of this PR, I'm just fetching the media inside the component.