Try adding dynamic page templates to pages section #50630
Conversation
|
Size Change: +844 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Thanks for putting this together.
Agree this is super-weird, hopefully it's not too troublesome to handle? I think we're zeroing in on the overall design for this panel. Obviously we wont tackle everything here, but for context: 
In terms of this PR, a couple of requests:
Do you intend to handle static home page + posts page in this PR? If so, the static homepage would appear first in the list, with the home icon. The posts page would appear next, with the 
|
|
Thanks for the updates we're getting close! When the homepage is set to display latest posts The Home item shares its title with the corresponding template, IE Front Page -> Home -> Index, which I think works well. The back button works as expected, and the posts page is showing the The remaining items seem to be:
|
I thought I'd done that, but didn't take into account that the pages returned are only the most recent. Looking into it now! |
|
To clarify, do we want to limit the amount of pages we display in addition to home/blog/404/search to only the x most recent? And how many would that be? |
Ok, I'm not sure I got this right, it feels a bit weird: sidebar.movI've also set all the pages to display for now, but happy to limit the amount again if needed. |
Yes, we don't have to handle that here though, unless it's trivial. I updated #44461 with the latest designs and some additional tasks, the aforementioned limit being one of them.
Sorry, I should have been more explicit. 404 / Search should be beneath the Manage All item. But since the latest designs reposition that item in the panel header, perhaps it's not a problem? The spacing looks a little off – there's a larger gap between 404/search compared to other pages. I'll see if I can push some tweaks. |
|
I pushed a couple of changes. It's still a bit wonky with the Manage link in it's current placement, but once we remove that everything will be fine: pages.mp4The only other tiny detail: I don't think we need the |
There was a problem hiding this comment.
Gave this a run through after looking through all the comments. It's working well for me and goes back to the pages list using both the back chevron and browser back button.
One thing to note, the transition animations don't work well in Safari, but not related to this PR.
|
Fixes #50582. Right? 🎉 |
I don't see any changes to this branch, did you push them to the remote? Given that a wider approach to the back button behaviour is being discussed in #50676, I'm going to revert the hacky fix for it in this PR. |
|
@jasmussen this resolves the pages list but still need for the page detail which will be added here |
|
Flaky tests detected in a151d40. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5107560423
|
I forgot to push 🙈 This is working well for me outside of two details:
|
|
I just made those changes, this feels like it's in a decent spot now. What do y'all think? |
| .edit-site-sidebar-navigation-screen-pages__see-all { | ||
| /* Overrides the margin that comes from the Item component */ | ||
| margin-top: $grid-unit-20 !important; | ||
| .edit-site-sidebar-navigation-screen__sticky-section.edit-site-sidebar-navigation-screen__sticky-section { |
There was a problem hiding this comment.
Should we move this here? And probably have a new property(stickyContent or whatever) in SidebarNavigationScreen like we have for content?
There was a problem hiding this comment.
It would be good to standardise this behavior if possible. We need it in the Page Details panel too, and likely others. You can see Saxon's comment here for the general sidebar spec.
There was a problem hiding this comment.
For consistency, I've used this sticky CSS over in the individual page details PR to style the last modified date element — thanks for the pointer @jameskoster — and have moved the style declaration to packages/edit-site/src/components/sidebar-navigation-screen/style.scss as per @ntsekouras comment
There was a problem hiding this comment.
These styles might not be generalisable as-is, especially the gap, unless we're sure they're only ever going to be applied to flex containers.
There was a problem hiding this comment.
I'm going to leave this as is for now, and revisit once #50767 is merged.
There was a problem hiding this comment.
I'm going to leave this as is for now, and revisit once #50767 is merged.
Merged!
There was a problem hiding this comment.
Thanks for the PR @tellthemachines! I've left some code comments for now, but my sole comment for the UX is that I found a bit off going to templates when I click back, after clicking the home page(that is actually a template).
| const homePageIndex = pages?.findIndex( | ||
| ( item ) => item.id === frontPage | ||
| ); | ||
| const homePage = pages?.splice( homePageIndex, 1 ); |
There was a problem hiding this comment.
splice is a mutating method, which means that changes the pages from store. In subsequent calls with the same args(default order), this mutated object will be returned.
There was a problem hiding this comment.
Is there any downside to that? If the pages are already in the correct order, this logic shouldn't change anything.
There was a problem hiding this comment.
The downside is:
In subsequent calls with the same args(default date order), this mutated object will be returned.
Someone could use the same underlying query in store from this call:
useEntityRecords( 'postType', 'page', {
per_page: -1,
} );
in a different component and will get this updated object, where it should still return the default ordering.
The solution could be simple enough by creating a swallow copy of pages.
There was a problem hiding this comment.
Got it, will update!
|
|
||
| const isHomePageBlog = frontPage === postsPage; | ||
|
|
||
| if ( ! isHomePageBlog ) { |
There was a problem hiding this comment.
Code here needs pages to have resolved, so we can add this in the if condition.
packages/edit-site/src/components/sidebar-navigation-screen-pages/index.js
Show resolved
Hide resolved
| withChevron | ||
| > | ||
| { decodeEntities( | ||
| item.title?.rendered |
There was a problem hiding this comment.
Can we end up with no title in these templates? 🤔
There was a problem hiding this comment.
I don't think so because the dynamic templates should always have a name.
tellthemachines
force-pushed
the
add/dynamic-templates
branch
from
5b9911d
to
0be138e
Compare
|
Thanks for reviewing @ntsekouras !
Yeah, I reverted the temporary fix I had for that because it's a wider issue that needs to be addressed as a whole. It's being discussed over in #50676. |
tellthemachines
force-pushed
the
add/dynamic-templates
branch
from
4a62a9b
to
2ba722b
Compare
|
Ok I think that's all the feedback addressed now! This is ready for another round. |
There was a problem hiding this comment.
Looking pretty good!
Displays my homepage correctly. And when I change my reading settings, the correct icons are shown next to the home page and posts page.
I'm also wondering about how the navigation should work. Templates return me to the templates list, and pages to the pages list.
I know this is the way the route logic is set up, but it seems inconsistent.
edit: Oh I see it's being discussed. I missed that, sorry!
2023-05-29.13.41.17.mp4
Maybe a follow up if we decide to force a return to the page list?
packages/edit-site/src/components/sidebar-navigation-screen-pages/index.js
Show resolved
Hide resolved
| </Truncate> | ||
| </PageItem> | ||
| ) } | ||
| { reorderedPages?.map( ( item ) => { |
There was a problem hiding this comment.
Optional, but perhaps this could be simplified with destructuring, and a function to grab the icon.
const getIcon = ( id ) => { // I don't think we need useCallback here since `frontPage` and `postsPage` won't change after defined.
if ( id === frontPage ) {
return home;
}
if ( id === postsPage ) {
return loop;
}
return page;
};
....
{ reorderedPages?.map( ( { id, title } ) => (
<PageItem
postId={ id }
key={ id }
icon={ getIcon( id ) }
withChevron
>
<Truncate numberOfLines={ 1 }>
{ decodeEntities( title?.rendered ) ??
__( '(no title)' ) }
</Truncate>
</PageItem>
) ) }| page.title?.rendered | ||
| ) ?? __( 'Untitled' ) } | ||
| item.title?.rendered | ||
| ) ?? __( '(no title)' ) } |
There was a problem hiding this comment.
#50767 uses 'Untitled' as fallback.
I have no preference either way, but we should make them the same.
There was a problem hiding this comment.
"no title" is what the pages were already using, this PR only adds a couple more instances. Was changing it to "Untitled" a design requirement? I'm happy with either, but would prefer to address that as a follow-up.
There was a problem hiding this comment.
Was changing it to "Untitled" a design requirement?
Not at all, just asking so that I can update the individual page view to match this copy 👍
ndiego
added
[Feature] Site Editor
* Try adding dynamic page templates to pages section * remove recent heading in pages * Static homepage case * Back button should go back to Pages * Fix order of home and blog pages * Try fetching all the pages * Stick templates to bottom * Stick item positioning * Revert custom back button behaviour * Move manage link to dynamic pages group * Remove home/posts page suffixes * Address feedback * Add back truncation * Copy array for reorder * Consolidate sticky styles --------- Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> Co-authored-by: James Koster <james@jameskoster.co.uk>
What?
Closes #50418.
Adds 404, Search and Homepage if it's set to posts, or Homepage and Blog page if home is set to a static page.
Back buttons are now working (clicking back from a template accessed from the Pages list should go back to Pages). Would appreciate feedback on the implementation though as there might be a better way of doing it.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast