Site editor sidebar: add page details #50767
Conversation
|
Size Change: +2.23 kB (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
|
Is it a bit strange to see the featured image in the sidebar, if the assigned template doesn't display it? 🤔 |
ramonjd
force-pushed
the
update/page-detail
branch
from
dcabb2e to
5207277
Compare
|
I've added a commit 5207277 Some notes:
🍺 |
| ).toBe( '2 days ago' ); | ||
| } ); | ||
|
|
||
| it( 'should return human readable time differences in the future', () => { |
There was a problem hiding this comment.
I just added some tests to make sure it can handle future dates.
packages/edit-site/src/components/sidebar-navigation-screen-pages/index.js
Show resolved
Hide resolved
| const wordsCounted = page?.content?.raw | ||
| ? wordCount( page.content.raw, wordCountType ) | ||
| : 0; | ||
| const readingTime = Math.round( wordsCounted / AVERAGE_READING_RATE ); |
There was a problem hiding this comment.
How useful are wordcount and reading time stats to users in this context?
There was a problem hiding this comment.
Good question — I quite like the inclusion, it could be useful if you're wanting to see at a glance how long a really long page might be, but I imagine it might be more useful for posts?
ramonjd
added
[Type] Enhancement
|
Flaky tests detected in 426bb19ed9abbe32eb418f369abecde5354f42ba. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5078516262
|
|
@ramonjd The last-modified detail is important, whether here or in a follow-up, as it will serve as an access point to revisions. I'd say it's nice to include the Featured Image if we can get the appearance right. I can still see folks being a bit confused if they set it, and see no change in the frame. But some help text that clarifies where it can be used would probably address that problem. Excerpt seems more useful for posts. And we still need to figure out how editing would work. So I wouldn't mind leaving them out for now. Not a strong feeling though. 
|
|
Thanks for the feedback @jameskoster
No worries. Just to confirm, we have the modified date from the post object, but to get the author who made the revision we'll need to fetch the revisions. I can look into this separately so it's easier to test. 👍
No worries. Let's leave it in. Perhaps we can center the image within the container, but I guess it depends on the container. Not sure how to ensure it will appear 100% of the time.
It will rarely show anyway so no harm in leaving it in. |
Oh, nice. The author is less important, fine to handle that one separately. Here are some outstanding details, and a refreshed mockup.
Thanks! I'm try to handle some of the other styling details when we get a bit closer. |
Oh, I didn't realize there were mock ups. I'm looking at this screenshot. I'll see how far I get with CSS + icons, but it might be useful to have SVGs if I run into geometrical challenges. Cheers!
I tried that yesterday. It might entail some restructuring of the HTML. Will have another crack. Edit: I can't find a neat way to do this within the constraints of For now I've stretched the flex item and rendering the modified date link beneath the items but with a bit of padding for good luck. |
ramonjd
force-pushed
the
update/page-detail
branch
from
4f88b27 to
c578057
Compare
|
It might be worth looking at how the 'dynamic pages' group is handled in #50630 for implementation? It's basically the same thing. Here are the svgs: Published Draft Pending / Scheduled |
|
Great, thanks for the PNGs. Nice!
It which sense? The sticky footer? I'll have a closer look and see if it's possible in this case. I'd submit it isn't a blocker for MVP. Edit: Looks like it implements something I already tried and cast aside because it overlaps the details as the window height reduces - if you're happy with that I'll reinstate. At least things will be consistent and we can always tweak later. Only sticking the element to the bottom of an 100% container and catering for resize + scroll was tripping me up. Cheers again!! 🙇 |
ramonjd
requested review from
noisysocks,
tellthemachines,
andrewserong and
jameskoster
There was a problem hiding this comment.
This is looking great to me, nice work folks!
Overcome your bedazzlement and provide some constructive feedback.
I was mostly bedazzled — it's looking very cool indeed, but I left a few nits. The only issue I ran into is that it looks like some of the labels might need to be wrapped in translate calls, unless I'm missing something?
Visually it's looking and testing great for me 🎉
| Published | Draft with image | Scheduled with parent |
|---|---|---|
 |
 |
 |
packages/edit-site/src/components/sidebar-navigation-data-list/style.scss
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-page/index.js
Outdated
Show resolved
Hide resolved
| const wordsCounted = page?.content?.raw | ||
| ? wordCount( page.content.raw, wordCountType ) | ||
| : 0; | ||
| const readingTime = Math.round( wordsCounted / AVERAGE_READING_RATE ); |
There was a problem hiding this comment.
Good question — I quite like the inclusion, it could be useful if you're wanting to see at a glance how long a really long page might be, but I imagine it might be more useful for posts?
packages/edit-site/src/components/sidebar-navigation-screen-page/style.scss
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-pages/index.js
Show resolved
Hide resolved
|
RTL 
|
- exports `getMediaDetails()` from the post-featured-image sidebar component and uses it if there's a featured image in the post record - cleans up components and unused functions - uses available wordcount and readtime functions - implements i18n strings - shuffles page-specific styles around - update package log after importing @wordpress/wordcount - truncating titles and other long excerpts - adding status icons - adding featured image description and loading placeholder
Removing duped/unused test expectations De-nesting BEM rules Remove unused comments i18n
De-nesting BEM rules Remove unused comments i18n
Importing getFeaturedMediaDetails into privateApis for the editor package. Removing unnecessary conditional checks Escaping HTML and other values for rendering in page and attributes - adding escape-html to package.json Don't render details when we don't have a record
ramonjd
force-pushed
the
update/page-detail
branch
from
426bb19 to
7287a73
Compare
|
Thanks for reviewing @ntsekouras ! I've made those changes and rebased. I think this should be good for a final test and then 🚀 |
There was a problem hiding this comment.
This is still testing well for me! Just found a tiny issue that private pages shows up with an empty status field, so left a comment there.
Thanks for the PR! Is this how it's supposed to look or is it something with my local cache or something? 😄
I'm saying that because @andrewserong screenshot has different alignments(right).
It looks like the designs were updated since I last reviewed, and the PR appears to be consistent with the designs shared in #50390. I think personally, I slightly prefer the right aligned look, but don't feel too strongly about it.
One potential issue is with how each of the value fields look when the data inside is wrapped. To my eyes the scheduled date in the following looks a little odd because it's center aligned vertically, so the value is visually higher than the detail label:
This is quite fine-tuning stuff, though, so personally I wouldn't think either of the above issues as blockers to landing if it's easier to get the larger PR in, and then tweak things in follow-ups.
| return ( | ||
| <div | ||
| className={ classnames( | ||
| 'edit-site-sidebar-navigation-screen-page__status', | ||
| { | ||
| [ `has-status has-${ status }-status` ]: !! status, | ||
| } | ||
| ) } | ||
| > | ||
| { statusIcon } { statusLabel } | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
It appears we render an empty Status area if a page is set to "private":
Should we add a label for private status? And if there's no match for status, would it be better to hide the whole row, or leave blank as it is currently?
There was a problem hiding this comment.
I believe we should hide the whole row if we determine a status, thanks for confirming this. I'll update.
There was a problem hiding this comment.
Actually 'private' is a status, but we don't cater for it in terms of icon or copy
There was a problem hiding this comment.
Also, what about password protected? We can detect that as well. Perhaps we need more icons?
I've done this for now:
There was a problem hiding this comment.
Yeah, for private posts/pages, I was imagining it could just be a Private status label with no icon, a little bit like the pending status for now.
But also, feel free to leave it for a follow-up — this PR is already fairly large, so it could be easier to fine-tune / fix up these things in follow-ups if you'd prefer. Whichever works best for you!
There was a problem hiding this comment.
Sorry, missed your reply before commenting. Those work nicely for me!
There was a problem hiding this comment.
Thanks! Icons are easy to add in a follow up. Let's merge this!
I'll leave this to @jameskoster or @SaxonF 😄 |
|
I think we should merge before this PR gets too hairy. Follow ups will be a cinch. 🍺 |
Agreed. Nice work on this PR! 🎉 |
I'm not sure it's related to this PR as it's pretty contained in the page panel. I can't replicate? |
| * @param {number} postId The post ID of the media | ||
| * @return {MediaDetails} The featured media details. | ||
| */ | ||
| export default function getFeaturedMediaDetails( media, postId ) { |
There was a problem hiding this comment.
It is very unclear to me why this is a selector of "editor" package instead of being a "hook" in edit-site for instance. editor should never be imported in edit-site IMO.
There was a problem hiding this comment.
Looking further at the PR, I guess we want to reuse the same functionality that was present in this component. I think "core-data" is a more suited place for this and I'm also not sure whether it should be a hook or selector.
There was a problem hiding this comment.
There is the option to revert this entirely and use the medium image size, and where that's not available the full one in the sidebar.
The image dimensions are constrained and cropped by CSS anyway so, image download size aside, the width and height are not as important since we're not displaying the entire image as we are in the block inspector.
There was a problem hiding this comment.
There is the option to revert this entirely and use the medium image size, and where that's not available the full one in the sidebar.
Yeah I can't tell why we had the logic in the sidebar in the first place, it's also a weird logic: runs a filter using the "large" size, otherwise run the same filter using the "thumbnail" size. There's probably some historic reasons for these filters to be required.
| return { | ||
| parentTitle: _parentTitle, | ||
| templateSlug: getEditedPostContext()?.templateSlug, | ||
| featuredMediaDetails: { |
There was a problem hiding this comment.
This component is going to be re-rendered on each store change. We shouldn't return random objects from useSelect callbacks. We should have a lint rule about that ideally.
|
|
||
| export default function SidebarNavigationScreenPage() { | ||
| const { setCanvasMode } = unlock( useDispatch( editSiteStore ) ); | ||
| const { getFeaturedMediaDetails } = unlock( privateEditorApis ); |
There was a problem hiding this comment.
Why is this called within the component instead of top level?
| ? decodeEntities( _parentTitle.title.rendered ) | ||
| : __( 'Untitled' ); | ||
| } else { | ||
| _parentTitle = __( 'Top level' ); |
There was a problem hiding this comment.
I'm not sure returning translated strings within useSelect is a good idea? Not sure if we've done this in the past. Translations feel more like "leaf" things and they can be filtered as well.
| const { records: templates, isResolving: areTemplatesLoading } = | ||
| useEntityRecords( 'postType', 'wp_template', { | ||
| per_page: -1, | ||
| } ); |
There was a problem hiding this comment.
Do we really need to fetch all the templates, to retrieve one by slug, can't we call a single getEntityRecord instead?
There was a problem hiding this comment.
I had checked this too. We cannot make a request for templates by slug if I'm not wrong. That's something we should address though at some point.
There was a problem hiding this comment.
I think we can, for templates the theme//slug is the id
There was a problem hiding this comment.
I also tried getEntityRecord and it does return slug, but only the template name segment, e.g.,
{
"postType": "page",
"postId": "2",
"templateSlug": "page"
}So I needed a way to get the current theme to build the id, e.g., twentytwentythree//${ _templateSlug }
There was a problem hiding this comment.
And now I've just discovered getCurrentTheme() 
There was a problem hiding this comment.
It might be good to check how we're retrieving the "post/page" template in the sidebar of the post editor. It's the same issue here.
There was a problem hiding this comment.
Nice!
That's the first place I looked but it's using availableTemplates, which is a property of select( editorStore ).getEditorSettings(), but now I see the site editor settings have it under defaultTemplateTypes. 👍 Updated.
| Details | ||
| </SidebarNavigationSubtitle> | ||
| <SidebarDetails | ||
| details={ getPageDetails( { |
There was a problem hiding this comment.
This function is a bit weird, Can't we instead transform it to a <PageDetails id={pageId} /> instead and collocate all the logic?
| d="M14.5 8a6.5 6.5 0 1 1-13 0 6.5 6.5 0 0 1 13 0ZM16 8A8 8 0 1 1 0 8a8 8 0 0 1 16 0ZM8 4a4 4 0 1 0 0 8 4 4 0 0 0 0-8Z" | ||
| /> | ||
| </SVG> | ||
| ); |
There was a problem hiding this comment.
@jasmussen Are we ok with all these adhoc icons? Should we add them to the icons library?
There was a problem hiding this comment.
They're specific to the page details, but happy to move them to the icons library. I have a refactor PR going at #51093
| </SVG> | ||
| ); | ||
|
|
||
| export default function StatusLabel( { status, date } ) { |
There was a problem hiding this comment.
Should we extract this one to its own file/folder? Feels more like a reusable UI component?
There was a problem hiding this comment.
I think we could, but maybe should wait until there's a use case? At the moment it's 100% specific to pages so I reckon we could keep in contained within the suggested PageDetails until there's a need?
There was a problem hiding this comment.
I'm not saying extract it to the "components" package yet but the file looks a bit crowded to me. But it's minor, fine to wait.
Thanks for looking at it @youknowriad I'll make a follow up tomorrow to address your feedback. |
|
First stab at refactor PR over at #51093 |
What?
Updates the existing page detail view in the site editor to include more useful information.
Why?
#50390
Constraints
Testing Instructions