Site editor sidebar: refactor page details #51093
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Refactors page details and the useSelect callbacks - Rolls back the generic data list components since we don't need them anywhere else (premature optimization)
ramonjd
added
[Type] Code Quality
ramonjd
requested review from
youknowriad,
SaxonF,
tellthemachines,
andrewserong and
ntsekouras
|
Size Change: +155 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
May 30, 2023
packages/edit-site/src/components/sidebar-navigation-screen-page/index.js
Outdated
Show resolved
Hide resolved
Closed
andrewserong
approved these changes
May 31, 2023
There was a problem hiding this comment.
Thanks for the follow-up @ramonjd! Just left a totally non-blocking comment about the getPageDetails function and whether it could be removed. Feel free to ignore, as I think it mostly comes down to a code style preference one way or another. This change is all testing nicely for me:
✅ Draft, published, scheduled, and private pages are all displaying correctly, and the details fields all appear to display as on trunk to me
✅ The useSelect usage in SidebarNavigationScreenPage and PageDetails looks cleaner now, without nested objects 👍
✅ Reverted getMediaDetails appears to be restored correctly, and the featured image behaviour in the sidebar in the editor looks correct
LGTM! ✨
Comment on lines
+142
to
+146
| { getPageDetails( { | ||
| parentTitle, | ||
| templateTitle, | ||
| ...record, | ||
| } ).map( ( { label, value } ) => ( |
There was a problem hiding this comment.
Not a blocker for this PR, as I think this PR already rearranges things quite nicely, but I was wondering if having a separate getPageDetails function is necessary if we have a PageDetails component?
Another way to write it could be to have some sub-components like DetailRow and DetailLabel and DetailValue (still in the same file), and then instead of iterating over the results of the imperative getPageDetails, we could compose the structure of the page details directly with React components?
Perhaps something like:
<DetailsRow>
<DetailsLabel>{ __( 'Status' ) }</DetailsLabel>
<DetailsValue>...</DetailsValue>
</DetailsRow>
{ !! page?.templateTitle && (
<DetailsRow>
<DetailsLabel>{ __( 'Template' ) }</DetailsLabel>
<DetailsValue>{ decodeEntities( page.templateTitle ) }</DetailsValue>
</DetailsRow>
) }
...
I wouldn't worry about it at all for this PR, and it quite possibly comes down to personal preference — personally, I find reading the React-ey output a little easier to follow than reading the constructed array in getPageDetails, but this is pretty nit-picky, so do feel free to ignore 😄
There was a problem hiding this comment.
Thanks for the review, and for thinking about how to refactor the details.
Yeah, now that you've written it out that way, I tend to agree it's easier to grok where there are components.
I'm not sure yet, but I believe that templates/template parts may get the same details treatment as pages. If that occurs then there might be an obvious path for optimization according to the way you've described.
There was a problem hiding this comment.
I'm not sure yet, but I believe that templates/template parts may get the same details treatment as pages. If that occurs then there might be an obvious path for optimization according to the way you've described.
Absolutely, I think it's perfectly fine to wait until there's a clearer path for subsequent usage 👍
|
Thank you Ramon for the follow up! |
This was referenced Jul 10, 2023
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* - Rolls back exposing getMedia details from the editor package - Refactors page details and the useSelect callbacks - Rolls back the generic data list components since we don't need them anywhere else (premature optimization) * - Ensuring last modified is visible * Use `availableTemplates` from editor settings to fetch template title using slug * Checking for templateSlug before performing a find * Whoops forgot to extract props from featuredMedia object in useSelect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
The redesign for the site editor sidebar's individual page details were introduced in #50767.
This PR refactors the code by:
getMediaDetailsfrom the editor package and using fetched attachment media props insteadKudos to @youknowriad and @ntsekouras for raising most of these improvements
Why?
useSelectis returning only what the components needs and doesn't return__()Testing Instructions
There should be no regressions.
Screenshots
Trunk
This PR