Add header to the quick edit when bulk editing #67390
Conversation
|
Size Change: +381 B (+0.02%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
louwie17
requested review from
youknowriad,
oandregal,
jameskoster and
gigitux
|
@jameskoster here is the initial header for bulk editing without the ellipsis action menu. Let me know if you have any styling suggestions. |
| className="edit-site-post-edit-header__title" | ||
| weight={ 500 } | ||
| as="h2" | ||
| lineHeight="20px" |
There was a problem hiding this comment.
Sounds good, I updated that as part of: f35707b
I did have to update the mixins though, as they were relying on a line height variable that didn't exist, let me know if I updated it correctly: https://github.com/WordPress/gutenberg/pull/67390/files#diff-96351000444fb28f5c80a41909d185066c8202d23cdb19c69fc21f79f76960e8L18-R64
Also I did mimic the above from the PostCardPanel component
heading-medium() as well?
There was a problem hiding this comment.
as they were relying on a line height variable that didn't exist
Oh good catch :D Thanks for fixing that. I guess this is the first time we're using the mixins :)
would it make sense to update that header to use the heading-medium() as well?
Yes I think they should be the same.
| } | ||
|
|
||
| .edit-site-post-edit-header__title { | ||
| padding: 2px 0; |
There was a problem hiding this comment.
Is this to align the icon and title? I think we can do that by adjusting the align property on the HStack, then remove this style.
There was a problem hiding this comment.
Yeah, and thanks for calling out the use of align. Align center allows me to remove the above :)
|
This is working well :) I left a couple of small comments. We might consider using |
|
Random observation, on this branch I'm not seeing the 'Homepage' and 'Posts page' badges next to the relevant page titles. Unsure if it's related to the changes here or not? Seems to be fine on trunk: 
|
| ); | ||
|
|
||
| return { | ||
| icon: unlock( select( editorStore ) ).getPostIcon( postType, { |
There was a problem hiding this comment.
Weird place for such a selector (nothing related to this PR just an observation :P)
| const _record = getEditedEntityRecord( | ||
| 'postType', | ||
| postType, | ||
| ids[ 0 ] |
There was a problem hiding this comment.
Looks like this only depends on the first id, so we might not need the useMemo for ids at all
There was a problem hiding this comment.
Hmm we do still make use of the ids further down, both to decide if to show the single header or to show the count of records being edited.
Or do you mean not passing the entire ids list into the useSelect dependency array?
Btw I am happy to remove useMemo use, as split is a low effort task. I followed what was done in the PostEditForm component ( line ).
I could also pass in the list of ids into the Header component.
| @@ -0,0 +1,84 @@ | |||
| /** | |||
There was a problem hiding this comment.
Nit: I wonder if this deserves its own folder or if it could be just a header.js file within the post-edit.
Personally and given the CSS is smallish, I could have kept it in the same folder.
There was a problem hiding this comment.
Yeah I was toying with that in my head as well. I did move it to the post-edit folder: b4297a7
youknowriad
added
[Type] Enhancement
louwie17
force-pushed
the
add/67344_bulk_editing_header
branch
from
cc7ccb7 to
b4297a7
Compare
|
Thanks for the reviews @jameskoster and @youknowriad, I have addressed the suggestions and this should be ready for re-review.
@jameskoster how do I actually get these badges to show? I wasn't able to display them on |
Go to "Settings > Reading" and set the "Homepage" and the "Posts page" to a static page: 
|
Thanks @oandregal :) @jameskoster it did still work for me locally on this branch and
|
packages/base-styles/_mixins.scss
Outdated
| @@ -659,7 +670,7 @@ | |||
| outline-color: var(--wp-admin-theme-color); | |||
| outline-style: solid; | |||
| outline-width: calc(#{$widthRatio} * (var(--wp-admin-border-width-focus) / var(--wp-block-editor-iframe-zoom-out-scale, 1))); | |||
| outline-offset: calc(#{$widthRatio} * ((-1 * var(--wp-admin-border-width-focus) ) / var(--wp-block-editor-iframe-zoom-out-scale, 1))); | |||
| outline-offset: calc(#{$widthRatio} * ((-1 * var(--wp-admin-border-width-focus)) / var(--wp-block-editor-iframe-zoom-out-scale, 1))); | |||
| } | |||
|
|
|||
| @mixin selected-block-focus($widthRatio: 1) { | |||
There was a problem hiding this comment.
Are all these random CSS changes related to this PR?
There was a problem hiding this comment.
Looks like a lot of formatting changes got in, when fixing the mixins at the top of this file.
| } | ||
|
|
||
| return ( | ||
| <VStack spacing={ 1 } className="edit-site-post-edit-header"> |
There was a problem hiding this comment.
Part of me wonders if PostCardPanel should support multiple posts (as this seems to recreate a similar UI)
There was a problem hiding this comment.
Yeah I could move it there, I initially decided not to given that PostCardPanel was initially written for the single header within the site editor.
Also if we do add support, should it be moved to the editor package instead?
louwie17
force-pushed
the
add/67344_bulk_editing_header
branch
from
b4297a7 to
031625d
Compare
There was a problem hiding this comment.
A couple things I think we should consider doing:
- Moving the code to the "PostCardPanel" (and support multiple postIds there)
- Adding the actions dropdown (the actions already support multiple postIds, and I believe actions are used within PostCardPanel already)
That said, this is an improvement 👍
Thanks for the review @youknowriad, I am good with the two above and can do those as a follow up if that works?
|
|
This works well, thank you. There's the detail of the "header" and the "title field" to have the same information, which becomes too much when they have the badge: 
@jameskoster suggested removing the title field, and that seems sensible. It's not rendered in the DocumentInspector and there's already a "rename action" that enables users to rename it. Though, if we do so, users would no longer have access to rename titles in bulk:
I think this is fine. If we wanted that we could declare the action as a bulk action I presume. |
| @@ -64,7 +64,7 @@ export default function PostCardPanel( { | |||
| [ postId, postType ] | |||
| ); | |||
|
|
|||
| const pageTypeBadge = usePageTypeBadge(); | |||
| const pageTypeBadge = usePageTypeBadge( parseInt( postId, 10 ) ); | |||
There was a problem hiding this comment.
Why do we need to parse the postId to an integer? Isn't it already one? Or perhaps the previous getCurrentPostId returned an integer and the current method (through props somewhere) returns a string? I also see that we don't need to parse it in other places.
There was a problem hiding this comment.
Not in the PostEditHeader component where we render this, but it is an integer in the PostSummary.
I will remove the parsing here and move that up one level to the header. As we know its a string there for sure.
There was a problem hiding this comment.
Just noting that postId is sometimes a real string (templates and template parts). It's true that quick edit is only used for "posts" and "pages" now but PostCardPanel can be used for templates and template parts too.
There was a problem hiding this comment.
Yeah I moved it up to the PostEditHeader: 5e2d674
The usePageTypeBadge hook works fine whether passing an integer or string, but does rely on integer for the home/posts page match.
| @@ -18,25 +18,25 @@ | |||
| @mixin heading-small() { | |||
| @include _text-heading(); | |||
| font-size: $font-size-x-small; | |||
| line-height: $line-height-x-small; | |||
| line-height: $font-line-height-x-small; | |||
There was a problem hiding this comment.
Ah, nice catch. It looks like these weren't working since #65418 ?
There was a problem hiding this comment.
Yeah correct, although I don't think the Mixin's where being used anywhere within GB yet.
|
Thanks for the additional review @oandregal, I addressed your comment around the I will do a follow up PR on these items that @youknowriad had mentioned:
|
What?
This partially addresses #67344, by adding a header to the quick edit panel when we are quick editing multiple pages.
Follow ups
PostActions( link ) do we change this to add bulk support?DataViewbulk actions ( link )Why?
In line with the header we show for editing a single page, we should also show one for editing multiple pages.
How?
I created a
PostEditHeadercomponent that handles both the header for a single post/page or multiple.Incase of a single it renders the existing
PostCardPanel.Testing Instructions
Changes will be applied to all selected pages.descriptionTesting Instructions for Keyboard
Screenshots or screencast
quick-bulk-editing-header.mp4