-
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
Fix warnings within useSelect of the bulk editing header #67872
base: trunk
Are you sure you want to change the base?
Changes from 5 commits
aec7269
2840862
5762893
6972c79
fce61de
e4cd460
9e4ceb6
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 |
---|---|---|
|
@@ -891,6 +891,57 @@ export const getEditedEntityRecord = createSelector( | |
} | ||
); | ||
|
||
/** | ||
* Returns a list of entity records, merged with their edits. | ||
* | ||
* @param state State tree. | ||
* @param kind Entity kind. | ||
* @param name Entity name. | ||
* @param recordIds Record IDs. | ||
* | ||
* @return The list of entity records, merged with their edits. | ||
*/ | ||
export const getEditedEntityRecords = createSelector( | ||
< EntityRecord extends ET.EntityRecord< any > >( | ||
state: State, | ||
kind: string, | ||
name: string, | ||
recordIds: EntityRecordKey[] | ||
): Array< ET.Updatable< EntityRecord > | false > => { | ||
return recordIds.map( ( recordId ) => | ||
getEditedEntityRecord( state, kind, name, recordId ) | ||
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. Given I re-use 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.
That's probably true. What are our options? Should we use 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. Actually I stand corrected, 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. We can run permissions selector after items are resolved. This way we can avoid dispatching a permission request for each item. 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 can try, but I am skeptical that would work. Given the individual record calls are not triggered because they are resolved by 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. Ok, so the problem was that I was 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. It should. The
Maybe we should normalize IDs before passing them to selectors. 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.
Yeah not a bad plan, with the current changes I suppose we are doing a form of normalizing by relying on the ID type that the entity record has. |
||
); | ||
}, | ||
( | ||
state: State, | ||
kind: string, | ||
name: string, | ||
recordIds: EntityRecordKey[], | ||
query?: GetRecordsHttpQuery | ||
) => { | ||
const context = query?.context ?? 'default'; | ||
return [ | ||
state.entities.config, | ||
...recordIds.map( | ||
( recordId ) => | ||
state.entities.records?.[ kind ]?.[ name ]?.queriedData | ||
.items[ context ]?.[ recordId ] | ||
), | ||
...recordIds.map( | ||
( recordId ) => | ||
state.entities.records?.[ kind ]?.[ name ]?.queriedData | ||
.itemIsComplete[ context ]?.[ recordId ] | ||
), | ||
...recordIds.map( | ||
( recordId ) => | ||
state.entities.records?.[ kind ]?.[ name ]?.edits?.[ | ||
recordId | ||
] | ||
), | ||
]; | ||
} | ||
); | ||
|
||
/** | ||
* Returns true if the specified entity record is autosaving, and false otherwise. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,52 +39,50 @@ export const TemplateEdit = ( { | |
typeof data.id === 'number' ? data.id : parseInt( data.id, 10 ); | ||
const slug = data.slug; | ||
|
||
const { availableTemplates, templates } = useSelect( | ||
const { templates, allowSwitchingTemplate } = useSelect( | ||
( select ) => { | ||
const allTemplates = | ||
select( coreStore ).getEntityRecords< WpTemplate >( | ||
'postType', | ||
'wp_template', | ||
{ | ||
per_page: -1, | ||
post_type: postType, | ||
} | ||
) ?? []; | ||
const allTemplates = select( | ||
coreStore | ||
).getEntityRecords< WpTemplate >( 'postType', 'wp_template', { | ||
per_page: -1, | ||
post_type: postType, | ||
} ); | ||
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. Remove the |
||
|
||
const { getHomePage, getPostsPageId } = unlock( | ||
select( coreStore ) | ||
); | ||
|
||
const isPostsPage = getPostsPageId() === +postId; | ||
const isPostsPage = getPostsPageId() === postId.toString(); | ||
const isFrontPage = | ||
postType === 'page' && getHomePage()?.postId === +postId; | ||
|
||
const allowSwitchingTemplate = ! isPostsPage && ! isFrontPage; | ||
postType === 'page' && | ||
getHomePage()?.postId === postId.toString(); | ||
|
||
return { | ||
templates: allTemplates, | ||
availableTemplates: allowSwitchingTemplate | ||
? allTemplates.filter( | ||
( template ) => | ||
template.is_custom && | ||
template.slug !== data.template && | ||
!! template.content.raw // Skip empty templates. | ||
) | ||
: [], | ||
allowSwitchingTemplate: ! isPostsPage && ! isFrontPage, | ||
}; | ||
}, | ||
[ data.template, postId, postType ] | ||
[ postId, postType ] | ||
); | ||
|
||
const templatesAsPatterns = useMemo( | ||
() => | ||
availableTemplates.map( ( template ) => ( { | ||
name: template.slug, | ||
blocks: parse( template.content.raw ), | ||
title: decodeEntities( template.title.rendered ), | ||
id: template.id, | ||
} ) ), | ||
[ availableTemplates ] | ||
allowSwitchingTemplate | ||
? ( templates ?? [] ) | ||
.filter( | ||
( template ) => | ||
template.is_custom && | ||
template.slug !== data.template && | ||
!! template.content.raw // Skip empty templates. | ||
) | ||
.map( ( template ) => ( { | ||
name: template.slug, | ||
blocks: parse( template.content.raw ), | ||
title: decodeEntities( template.title.rendered ), | ||
id: template.id, | ||
} ) ) | ||
: [], | ||
[ templates, allowSwitchingTemplate, data.template ] | ||
); | ||
|
||
const shownTemplates = useAsyncList( templatesAsPatterns ); | ||
|
@@ -151,6 +149,10 @@ export const TemplateEdit = ( { | |
variant="tertiary" | ||
size="compact" | ||
onClick={ onToggle } | ||
accessibleWhenDisabled | ||
disabled={ | ||
value === '' && ! templatesAsPatterns.length | ||
} | ||
> | ||
{ currentTemplate | ||
? getItemTitle( currentTemplate ) | ||
|
@@ -159,14 +161,16 @@ export const TemplateEdit = ( { | |
) } | ||
renderContent={ ( { onToggle } ) => ( | ||
<MenuGroup> | ||
<MenuItem | ||
onClick={ () => { | ||
setShowModal( true ); | ||
onToggle(); | ||
} } | ||
> | ||
{ __( 'Swap template' ) } | ||
</MenuItem> | ||
{ templatesAsPatterns.length > 0 && ( | ||
<MenuItem | ||
onClick={ () => { | ||
setShowModal( true ); | ||
onToggle(); | ||
} } | ||
> | ||
{ __( 'Swap template' ) } | ||
</MenuItem> | ||
) } | ||
{ | ||
// The default template in a post is indicated by an empty string | ||
value !== '' && ( | ||
|
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.
Let's make this new selector private for now.
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.
Done: e4cd460