-
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
Try adding dynamic page templates to pages section #50630
Conversation
Size Change: +844 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave this as is for now, and revisit once #50767 is merged.
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.
I'm going to leave this as is for now, and revisit once #50767 is merged.
Merged!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, will update!
|
||
const isHomePageBlog = frontPage === postsPage; | ||
|
||
if ( ! isHomePageBlog ) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we end up with no title in these templates? 🤔
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.
I don't think so because the dynamic templates should always have a name.
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
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.
PR here: #51074
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.
LGTM, thank you @tellthemachines!
* 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