-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Data Module: Refactor media fetching to use the core data module #5707
Changes from 4 commits
4a1e75f
740eaa8
40ce288
e8cbece
2388e13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,9 +22,9 @@ import { | |
SelectControl, | ||
TextControl, | ||
Toolbar, | ||
withAPIData, | ||
withContext, | ||
} from '@wordpress/components'; | ||
import { withSelect } from '@wordpress/data'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -136,7 +136,7 @@ class ImageBlock extends Component { | |
} | ||
|
||
getAvailableSizes() { | ||
return get( this.props.image, [ 'data', 'media_details', 'sizes' ], {} ); | ||
return get( this.props.image, [ 'media_details', 'sizes' ], {} ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Observing that we're still pretty bound to the REST API structure of an entity with the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I was wondering if we should introduce "models" somehow |
||
} | ||
|
||
render() { | ||
|
@@ -297,14 +297,12 @@ export default compose( [ | |
withContext( 'editor' )( ( settings ) => { | ||
return { settings }; | ||
} ), | ||
withAPIData( ( props ) => { | ||
withSelect( ( select, props ) => { | ||
const { getMedia } = select( 'core' ); | ||
const { id } = props.attributes; | ||
if ( ! id ) { | ||
return {}; | ||
} | ||
|
||
return { | ||
image: `/wp/v2/media/${ id }`, | ||
image: id ? getMedia( id ) : null, | ||
}; | ||
} ), | ||
] )( ImageBlock ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,12 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { combineReducers } from 'redux'; | ||
import { keyBy } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { combineReducers } from '@wordpress/data'; | ||
|
||
/** | ||
* Reducer managing terms state. Keyed by taxonomy slug, the value is either | ||
|
@@ -37,6 +42,27 @@ export function terms( state = {}, action ) { | |
return state; | ||
} | ||
|
||
/** | ||
* Reducer managing media state. Keyed by id. | ||
* | ||
* @param {Object} state Current state. | ||
* @param {Object} action Dispatched action. | ||
* | ||
* @return {string} Updated state. | ||
*/ | ||
export function media( state = {}, action ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DocBlock 😄 |
||
switch ( action.type ) { | ||
case 'RECEIVE_MEDIA': | ||
return { | ||
...state, | ||
...keyBy( action.media, 'id' ), | ||
}; | ||
} | ||
|
||
return state; | ||
} | ||
|
||
export default combineReducers( { | ||
terms, | ||
media, | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import apiRequest from '@wordpress/api-request'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { setRequested, receiveTerms } from './actions'; | ||
import { setRequested, receiveTerms, receiveMedia } from './actions'; | ||
|
||
/** | ||
* Requests categories from the REST API, yielding action objects on request | ||
|
@@ -17,3 +17,14 @@ export async function* getCategories() { | |
const categories = await apiRequest( { path: '/wp/v2/categories' } ); | ||
yield receiveTerms( 'categories', categories ); | ||
} | ||
|
||
/** | ||
* Requests a media element from the REST API. | ||
* | ||
* @param {Object} state State tree | ||
* @param {number} id Media id | ||
*/ | ||
export async function* getMedia( state, id ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering if passing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, I'm not too sure how much we want to promote the use of See also: #5219 (comment) |
||
const media = await apiRequest( { path: `/wp/v2/media/${ id }` } ); | ||
yield receiveMedia( media ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not tracking any "is requesting" state at the moment because it's not really needed for our current use-cases. But I was wondering if we should have a separate Same here, I guess we'll see with use-cases while adding other resolvers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The original implementation in ffb0760 had a standalone Related: #5219 (comment) |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,8 @@ import apiRequest from '@wordpress/api-request'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { getCategories } from '../resolvers'; | ||
import { setRequested, receiveTerms } from '../actions'; | ||
import { getCategories, getMedia } from '../resolvers'; | ||
import { setRequested, receiveTerms, receiveMedia } from '../actions'; | ||
|
||
jest.mock( '@wordpress/api-request' ); | ||
|
||
|
@@ -30,3 +30,21 @@ describe( 'getCategories', () => { | |
expect( received ).toEqual( receiveTerms( 'categories', CATEGORIES ) ); | ||
} ); | ||
} ); | ||
|
||
describe( 'getMedia', () => { | ||
const MEDIA = { id: 1 }; | ||
|
||
beforeAll( () => { | ||
apiRequest.mockImplementation( ( options ) => { | ||
if ( options.path === '/wp/v2/media/1' ) { | ||
return Promise.resolve( MEDIA ); | ||
} | ||
} ); | ||
} ); | ||
|
||
it( 'yields with requested media', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like these tests. Nice work setting up the pattern @aduth It also shows that we could push it further by avoiding the mocks entirely (using |
||
const fulfillment = getMedia( {}, 1 ); | ||
const received = ( await fulfillment.next() ).value; | ||
expect( received ).toEqual( receiveMedia( MEDIA ) ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import deepFreeze from 'deep-freeze'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { getTerms, isRequestingTerms } from '../selectors'; | ||
import { getTerms, isRequestingTerms, getMedia } from '../selectors'; | ||
|
||
describe( 'getTerms()', () => { | ||
it( 'returns value of terms by taxonomy', () => { | ||
|
@@ -56,3 +56,21 @@ describe( 'isRequestingTerms()', () => { | |
expect( result ).toBe( true ); | ||
} ); | ||
} ); | ||
|
||
describe( 'getMedia', () => { | ||
it( 'should return undefined for unknown media', () => { | ||
const state = deepFreeze( { | ||
media: {}, | ||
} ); | ||
expect( getMedia( state, 1 ) ).toBe( undefined ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Seems like a separate test case to me ( |
||
} ); | ||
|
||
it( 'should return a media element by id', () => { | ||
const state = deepFreeze( { | ||
media: { | ||
1: { id: 1 }, | ||
}, | ||
} ); | ||
expect( getMedia( state, 1 ) ).toEqual( { id: 1 } ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import { __ } from '@wordpress/i18n'; | |
import { Button, Spinner, ResponsiveWrapper, withAPIData } from '@wordpress/components'; | ||
import { MediaUpload } from '@wordpress/blocks'; | ||
import { compose } from '@wordpress/element'; | ||
import { withSelect } from '@wordpress/data'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -38,15 +39,15 @@ function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, med | |
modalClass="editor-post-featured-image__media-modal" | ||
render={ ( { open } ) => ( | ||
<Button className="button-link editor-post-featured-image__preview" onClick={ open } > | ||
{ media && !! media.data && | ||
{ media && | ||
<ResponsiveWrapper | ||
naturalWidth={ media.data.media_details.width } | ||
naturalHeight={ media.data.media_details.height } | ||
naturalWidth={ media.media_details.width } | ||
naturalHeight={ media.media_details.height } | ||
> | ||
<img src={ media.data.source_url } alt={ __( 'Featured image' ) } /> | ||
<img src={ media.source_url } alt={ __( 'Featured image' ) } /> | ||
</ResponsiveWrapper> | ||
} | ||
{ media && media.isLoading && <Spinner /> } | ||
{ ! media && <Spinner /> } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if a media request returns 404 ? This is a concern with treating "empty" as equivalent to "request in progress". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm fine with this right now, we can revisit once/if we introduce request state |
||
</Button> | ||
) } | ||
/> | ||
|
@@ -96,14 +97,22 @@ const applyConnect = connect( | |
} | ||
); | ||
|
||
const applyWithAPIData = withAPIData( ( { featuredImageId, postTypeName } ) => { | ||
const applyWithAPIData = withAPIData( ( { postTypeName } ) => { | ||
return { | ||
media: featuredImageId ? `/wp/v2/media/${ featuredImageId }` : undefined, | ||
postType: postTypeName ? `/wp/v2/types/${ postTypeName }?context=edit` : undefined, | ||
}; | ||
} ); | ||
|
||
const applyWithSelect = withSelect( ( select, { featuredImageId } ) => { | ||
const { getMedia } = select( 'core' ); | ||
|
||
return { | ||
image: featuredImageId ? getMedia( featuredImageId ) : null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've changed the prop name from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Damn copy/paste |
||
}; | ||
} ); | ||
|
||
export default compose( | ||
applyConnect, | ||
applyWithAPIData, | ||
applyWithSelect, | ||
)( PostFeaturedImage ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will
image
be falsey?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the API loads the image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't see the object fallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this pull request, but why do we access
this.props.url
here? Should this not be from thenextProps
argument?