-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Site editor sidebar: home template details #51223
Conversation
Size Change: +1.54 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
b11a8c7
to
5b42868
Compare
packages/edit-site/src/components/sidebar-navigation-screen-template/home-template-details.js
Outdated
Show resolved
Hide resolved
5b42868
to
663c8b3
Compare
Edit: Probably fine to include, if we append some help text explaining the global nature of the settings: I know it's still a WIP, but I just wanted to note that in the Areas section, each entire row should act as a button, rather than only the name of the template part alone. Mockup: I suspect it might be worth cc'ing @ciampo and @mirka for any insight on how best to theme the input components. |
Thank you for the ping! While the |
Thanks for the updated designs @jameskoster !
Good idea, will do. 👍 |
5e61d7b
to
381cc00
Compare
I've removed "Last updated" changes because the current API does not support it. We can reinstate once this PR is merged (if it does): |
f7d0092
to
798edab
Compare
Flaky tests detected in 2ade33c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5317430391
|
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.
This is testing really well! Just a couple questions below (mostly me trying to understand this better)
@@ -42,6 +43,11 @@ function useTemplateTitleAndDescription( postType, postId ) { | |||
); | |||
} | |||
|
|||
let content = null; | |||
if ( record?.slug === 'home' || record?.slug === 'index' ) { |
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.
If this applies to all blog templates, shouldn't it include the Archives template too? Or should it only apply to Home, with Index as a fallback if Home doesn't exist? Settings such as "site title" only really make sense on Home I guess 🤔
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.
Good question! I wasn't really sure about this. Happy to take instructions from @jameskoster here 😄
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, because Archive and so on can display custom post types too. It may be that we add a local posts-per-page setting to to templates like Archive in the future, but let's start with the global stuff for now.
Settings such as "site title" only really make sense on Home I guess
The setting here should be Blog title, not Site title. And it should only be visible when a Posts Page is set. Updating the Blog title simply updates the title of the Posts Page.
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.
Updating the Blog title simply updates the title of the Posts Page.
@jameskoster Ah, that was a misinterpretation on my part, sorry!
I've updated this PR so that the "Blog title" input control only shows when a static page is selected as the posts page. The value of the input control updates the title of that page.
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.
Just reflecting on the above change, I think I was unclear about why we would want to update the title of the posts page from the home template, when the two could have no relationship. For example, if I select page X
as my posts page, it might use a page template and not the home or index templates.
Unless the intention is to surface general site settings?
The only other conclusion I could reach is we'd show these controls when we set a static page as the posts page and that static page uses the home/index template.
Anyway, let me know if I'm way off.
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 example, if I select page X as my posts page, it might use a page template and not the home or index templates.
Please feel free to double-check but I think the Posts page will always resolve to either Home or Index. Note that the template switching UI in the post editor does not allow you to change the template when editing this page;
I've updated this PR so that the "Blog title" input control only shows when a static page is selected as the posts page. The value of the input control updates the title of that page.
If the above is true, this sounds perfect!
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 think the Posts page will always resolve to either Home or Index.
Oh yes, good one! Another piece of the puzzle falls into place. 😄
I've updated this PR so that the "Blog title" input control only shows when a static page is selected as the posts page. The value of the input control updates the title of that page.
Just to confirm, and maybe you're aware of this already. Here's what I'm seeing:
- Select a page to act as the posts page (it uses the home page template in 2022 and 2023)
- Here's the page in question. The title is highlighted
- When updating the post page title, the only thing that will change (only after save) on the home page is the nav menu item.
packages/edit-site/src/components/sidebar-navigation-screen-template/home-template-details.js
Outdated
Show resolved
Hide resolved
ff630c6
to
670d75f
Compare
This is looking pretty good, thanks for all the hard work @ramonjd :) So, I think I have uncovered a bit of an issue in updating the global posts per page setting in this panel. Many themes (including twenty twenty-three) set Query Loop blocks to Consequently, updating the global posts per page setting here won't always update how many posts per page are displayed on this template, and that is going to make for a confusing experience 🙃 I'm very open to ideas here, but (if possible) one option could be to link the Posts per page control in the sidebar to the Query Loop in the template. There would be the following potential scenarios:
Here's a mockup: What do you think? A couple of smaller details:
|
I think I get why I was stumbling over the requirements, thanks for helping me understand @jameskoster I think my main problem was trying make the cognitive link between the specific home page template and overall site settings. For example, "Allow comments on new posts" doesn't have any direct relationship to the home page template being edited/viewed. I'm wondering if these things will cause more confusion unless we better communicate their intended effect.
So, when I open a homepage template in view mode, we'd want to control (and update reactively) query block attributes in the editor from the sidebar. Or in other words, update block attributes in real time from the site editor sidebar in "view" mode. It might be possible to drill down the template's block tree and find the first query block to display how many posts a home page query block displays. Even, use the I'm just wondering why we couldn't guide folks to open the editor and do this in "edit" mode? It's what the editor is set up for and what it does best.
I think this would only make sense to do on the individual page, and only when that page is set to display posts. The post title block appears only on the page template that is set as the posts page in the reading settings. So, taking 2023's home page template as an example, the "Mindblown: a blog about philosophy." heading is just a heading block. Updating it would require first finding, then updating that particular heading block's content. There could be 1 heading there could be 100 on a template and we wouldn't really know which heading acts as the "title". Plus, and this is just my personal opinion, to me it blurs the lines of "view" and "edit" modes to allow updating of block-specific attributes from the sidebar. This close to code freeze for 6.3, my vote would be to revisit this functionality. There's the risk of introducing something that half works or is buggy, if we can get things working at all. With all this in mind, and at this stage, I think we could display some info that we know about the home template, e.g., like the page details panel. For example, we can already display template areas that appear on the template, e.g., footer and header (already done) Furthermore, if a "home" template exists, and the reading settings are set to "homepage displays displays your latest posts", then we could display this. I'll try to play with a few things so we have options. |
Without the controls, here's something we could do now: The last updated footer should appear on all template and template part panels once #51362 is merged. The "This template is currently set as your homepage" label will only show if:
|
ba8dbcf
to
f8344e7
Compare
Tweaking CSS accordingly
We can reinstate these changes once it's merged (also adding the property to core-data/src/entity-types/wp-template-part.ts)
Updating translations for entity types when saving
switching over site title editing to posts page title editing
- adds a last updated footer (if the modified property is available) - removes all controls and addes details
Reinstate allow comments control Abstract last modified footer
…rom the home template
Use spin custom controls on the number control component Update changelog
ea8e9ad
to
a7d95ff
Compare
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.
Nice work @ramonjd, this is looking good to me!
The features were all testing well according to the designs and how it's been developed 👍
The main issue I ran into is that the Blog title
control feels a little edge-casey to me, in that when a posts page is set to the home page, the post title block doesn't appear to output anything within that template as far as I could tell. So, when using it, I didn't feel like I was updating the blog title, and I found it hard to draw the connection between this input and updating the title for the page itself. I see that the help text for the field explains that the title will appear in search results and when the page is shared on social media, but in terms of the UI, I found it difficult to understand how the feature is meant to be used. Since we're editing the Home template, as a user, my expectation is that Blog Title actually means Site Title. That could just be me, though!
The discussion surrounding that control could very well become a bit rabbit-holey, so I reckon it'd be a good idea to merge this PR in its current shape, and defer further exploration of the Blog Title control to follow-ups.
LGTM! ✨
lib/compat/wordpress-6.3/class-gutenberg-rest-templates-controller-6-3.php
Outdated
Show resolved
Hide resolved
} from '../sidebar-navigation-screen-details-panel'; | ||
|
||
export default function SidebarNavigationScreenDetailsFooter( { | ||
lastModifiedDateTime, |
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.
Tiny nit: again not-blocking, but since this is a generic footer component, I was wondering if the check for whether or not to render the last modified row should exist within this component instead?
Just a thought though, and I wouldn't worry about changing it in this PR.
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.
Excellent. I'll add it to the imminent follow-up
useEffect( () => { | ||
setCommentsOnNewPostsValue( allowCommentsOnNewPosts ); | ||
setPostsPageTitleValue( postsPageTitle ); | ||
setPostsCountValue( postsPerPage ); | ||
}, [ postsPageTitle, allowCommentsOnNewPosts, postsPerPage ] ); |
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.
This syncs the server-retrieved values to local state, yeah? Only if you're making further changes anyway, would it be worth adding a comment just before this useEffect? It took me a second to work out what was happening.
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 worth it. Added to follow up notes. Thank you!
packages/edit-site/src/components/sidebar-navigation-screen-page/page-details.js
Show resolved
Hide resolved
* Initial commit: - refactoring page details so that the styles and components can be used in templates - getting home template details in order * - displaying template areas - refactoring footer to show last modified in template and page details * - bare bones rest controller changes to return modified for `get_item` - linking up template areas * - passing footer class to row * - hooking into settings. * Refactoring input controls layout Tweaking CSS accordingly * Reverted prefix change to file that was not copied to GB * Removing last modified changes until the templates API supports it. We can reinstate these changes once it's merged (also adding the property to core-data/src/entity-types/wp-template-part.ts) * Showing the details pages for the index template Updating translations for entity types when saving * Fixed new unlock path * updating design of area buttons switching over site title editing to posts page title editing * Updated hover states of area buttons * This commit: - adds a last updated footer (if the modified property is available) - removes all controls and addes details * Don't need these * Reinstate post title and posts per page controls Reinstate allow comments control Abstract last modified footer * Updated copy * Update help text * SidebarNavigationItem instead of button for links to template parts from the home template * Wrap areas in ItemGroup * Remove bottom margin on last detail panel * Spacing * Large inputs * Leave border radius on inputs as 2px for now * Use NumberControl * Remove debounce Use spin custom controls on the number control component Update changelog * Restore since annotation change made in WordPress#51362 --------- Co-authored-by: James Koster <james@jameskoster.co.uk>
What?
Displays template and settings configuration details for the home and index templates.
See the designs and specs over at:
Status
Testing Instructions
With a block theme activated, head over to the site editor and open the home or index template.
Check that the settings controls work, and that the template area links open the correct template:
/wp-admin/options-reading.php
/wp-admin/options-reading.php
, the post title control should display. Updating this field and saving should set the title of the posts page only.Screenshots or screencast
State of this PR: