Chrome: Adding a ResponsiveImage component to wrap the featured image #911
Conversation
youknowriad
added
the
General Interface
There was a problem hiding this comment.
Cool 👍 So to be clear, neither the done nor fail case are invoked when the request is aborted?
There was a problem hiding this comment.
I think so, I tested once but Let me test against to be absolutely certain :)
There was a problem hiding this comment.
Good catch @aduth the abort was being triggered :( So, now I'm ignoring aborted request in the fail callback.
youknowriad
force-pushed
the
add/responsive-image
branch
from
c1c18e0 to
dda7322
Compare
There was a problem hiding this comment.
I dislike acronyms, especially e, because it takes me a moment to distinguish intent between the event and error common use-cases. I'm always happier with the verbose equivalent.
There was a problem hiding this comment.
Let's make Andrew Happy 🙂. You're absolutely right, this is a bad habit of mine
youknowriad
force-pushed
the
add/responsive-image
branch
from
b26b340 to
2a872d7
Compare
| .fail( () => { | ||
| if ( this.props.featuredImageId !== mediaIdToLoad ) { | ||
| .fail( ( error ) => { | ||
| if ( error && error.statusText === 'abort' ) { |
There was a problem hiding this comment.
Looking to the docs, the first argument is not the error, but the jqXHR instance:
http://api.jquery.com/jQuery.ajax/#jqXHR
Is it ever falsey?
components/responsive-image/index.js
Outdated
| return ( | ||
| <div className="components-responsive-image"> | ||
| <div style={ imageStyle } /> | ||
| <img src={ src } className="components-responsive-image__image" alt={ alt } /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
youknowriad
force-pushed
the
add/responsive-image
branch
from
2765064 to
2d054a6
Compare
|
Rebased here. @aduth do you have time to look at this again? |
| import './style.scss'; | ||
|
|
||
| function ResponsiveWrapper( { naturalWidth, naturalHeight, children } ) { | ||
| if ( ! children || Array.isArray( children ) ) { |
There was a problem hiding this comment.
I think we don't want to test children as an array, but rather Children.count > 1
https://facebook.github.io/react/docs/react-api.html#react.children.count
Or even Children.only if we don't mind being very noisy (thrown errors) with incorrect usage:
https://facebook.github.io/react/docs/react-api.html#react.children.only
props @aduth
addresses #894 (comment)