-
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
Pages: Add "Set as posts page" action #67650
Conversation
Size Change: +209 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
Flaky tests detected in 35cea15. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12294916692
|
Nice :) I miss an option to set the posts page as the homepage, but I appreciate we can't do that yet because it means the 'page' would vanish from the data view. More importantly for now... If you reset the homepage when a posts page exists, the modal doesn't inform the user about which page will become the homepage. This feels like an important detail to communicate. I noticed that performing this action creates this configuration in reading settings: Unless I'm mistaken, there aren't any practical scenarios where you'd want to configure your pages this way? I wonder... do we need this 'Reset homepage' action? If you want to change the homepage it's easy to perform 'Set as homepage' on another page. There's also something a bit uncanny about the 'Reset homepage' labelling which we can solve by removing that action. What do you think? One other tiny detail; can we adjust the order of actions so that 'Move to trash' is always last? We may need to follow up to add some separation in this menu, it's getting quite long. cc @fcoveram as you've been thinking about that. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I agree, maybe we don't need the 'Reset homepage' option at all, as it does feel a bit strange. I started with 'Unset homepage' (and 'Unset posts page'), but the 'unset' wording didn't feel right either. Or, perhaps instead of "Reset homepage", the option should be along the lines of "Reset homepage to latest posts", and this option both unsets the static homepage and posts pages and then sets the latest posts as the page on front. This could be explained to the user in a notice in the confirmation modal. Another option could be to have the "Your latest posts" option separate from these actions, as a toggle somewhere outside of the pages table.
Yes! Done in 2cc8058. |
Yeah the whole reset homepage flow needs a bit more thought imo, probably worth exploring that one separately, but I'm not sure it's really needed. To be honest I'm on the fence about 'Reset posts page' as well. This flow is essentially serving a scenario where the user no longer wants a blog on their site, but in that case I suspect that simply deleting the page is more intuitive. I'd welcome more feedback from @gutenberg-design on that. |
Currently in the Site Editor it looks like this: A Posts Page and a Homepage has been defined. This means that this PR will add inside the 3 dots contextual drop down menu. That seems natural. In addition one can also reset the homepage and posts page. A few things.... Currently in the Reading Settings one has to click to define Your homepage displays - Your latest posts. Defining Homepage and Posts page are then greyed out. One can not any longer set any of these. The Site Editor: The current process of defining a posts page removes the content of the page. Should a posts page contain the default Query Loop block that can be edited? If there was preexisting content should that still be available below the Query Loop block? So that the predefined Posts page still has the content available, but now also contains an editable Query Loop block above it. This way one does not loose the information one had earlier on the page but can choose to later on adjust/remove it. At the same time one can then customize the default Query Loop block to how one wants to see the Posts page. |
@jasmussen to clarify; you're advocating for not having the following actions:
And instead offering a dedicated flow for creating a posts page, where one doesn't exist (#63667)? If so I think it can work, and inherently clarify some of the points outlined by Paal, but we need to find the right design in 63667. |
Sorry I should be clearer. I'm advocating for choosing one of two paths.
I don't really have a strong opinion either way. I think the latter is better. But it's a taller undertaking, and the former works today, in this PR, and is not as bad in practice as it sounds. So it could be a matter of doing 1 for now, and then upgrading to 2 depending on need. |
Thanks for clarifying. I agree in general, but lean towards omitting the 'Reset' actions in both paths. The value-add is limited imo. |
// Save new posts page settings. | ||
await saveEditedEntityRecord( 'root', 'site', undefined, { | ||
page_for_posts: 0, | ||
show_on_front: isPageOnFrontSet ? 'page' : 'posts', | ||
} ); | ||
|
||
// This second call to a save function is a workaround for a bug in | ||
// `saveEditedEntityRecord`. This forces the root site settings to be updated. | ||
// See https://github.com/WordPress/gutenberg/issues/67161. | ||
await saveEntityRecord( 'root', 'site', { | ||
page_for_posts: 0, | ||
show_on_front: isPageOnFrontSet ? 'page' : 'posts', | ||
} ); |
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 know much about this, but wouldn't just saveEntityRecord
work correctly?
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 also don't know much about these functions, but I believe ideally just saveEditedEntityRecord
should be required, as that calls saveEntityRecord
. However, using saveEntityRecord
on its own does seem to work successfully, so maybe we could just use that function in these cases.
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 looked through the codebase some more at other uses of these functions, and saveEntityRecord
is often used on its own (rather than saveEditedEntityRecord
as I expected), so I've removed saveEditedEntityRecord
from both the "Set as homepage" and "Set as posts page" actions. They still work fine so I think this is a nice refactor!
Commit with these changes is here: 2cfd817.
createSuccessNotice( __( 'Posts page reset' ), { | ||
type: 'snackbar', | ||
} ); | ||
} catch ( error ) { |
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 this exception error actually possible? Because saveEntityRecord
doesn't seem to throw any exceptions by default.
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 it's nice to have the catch
block anyway, to catch any other errors (like a network error maybe). But I completely see your point, I don't think saveEntityRecord
throws any exceptions.
Should we discuss the reset option in a separate issue/PR to keep the scope of this PR as small as possible? |
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
I think it depends on the consensus around the paths described in this comment, and this PR can be updated to reflect these changes:
I would lean more towards the second option, keeping the set/reset Homepage in the context menu, and discuss the designs for the posts page settings in #63667 as a next step. My biggest concern with keeping all 4 options is the amount of new page actions being included in this menu.
I think this is one of the most confusing homepage states for a user when in the Editor. Perhaps we could have a separate UI just for the homepage somewhere on the Pages data view, which may be linked to the design we go with in #63667 (i.e. maybe there's an "Edit homepage settings" option in this dropdown menu). |
Following the existing logic of the labels. |
That makes sense to me. I'm not convinced about the value of the 'Reset...' actions. |
This makes sense, however, when the homepage is set to show the latest posts, there isn't a
I agree, I think the need for having the reset actions is probably an indicator of the missing UI around the homepage settings. In that case, I could reduce this PR to just adding the "Set as posts page" action, and remove both the "reset" actions for now, and then open up a new issue to continue discussing the next steps for the homepage settings. |
I think this will be related to the admin redesign, which is one of Phase 3 (See #1111). Eventually, I expect the functionality for the display settings to be implemented in the new Settings screen, but I think it will be a very long way before that happens. |
I've updated this so that it now only adds the "Set as posts page" action, and then a few other smaller changes:
This is ready for another review! 🙇 |
I'm hoping this is fixed with the latest commit in f60fa36, which uses a similar check to the homepage action to prevent a similar error we were seeing there. That flyout menu looks good and I like the grouping of these actions. Would the actions only be grouped when there was more than one of them, or would it still show if only either the homepage or posts page actions were available? I guess for consistency in the menus, it might be best if they were always grouped into the separate menu.
Yes agreed, but I'm also struggling to think of alternatives! Maybe "Set as blog 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.
Should we group the 'Set as...' actions in a flyout to tidy the menu up a bit?
I'm not sure if the current Actions API supports submenus🤔
@youknowriad @oandregal Do you know anything about this?
packages/editor/src/components/post-actions/set-as-posts-page.js
Outdated
Show resolved
Hide resolved
It's not supported right now, also hard to think of a good API for it. We could explore it if needed. |
Nice, it's working now :) If adding the flyout isn't trivial let's not worry about it. |
Thanks @jameskoster!
Yeah, it's not trivial, but it would still be nice to revisit it later. |
This one seems in a good spot to me, pending code review :) |
Thanks all! We had a few design-related discussions here which I think can continue over in #63667. |
What?
#65426 added the Set as Homepage action to Pages, so this is a follow-up which adds a "Set as posts page" action as well.
It also includes a few other smaller tweaks:
getItemTitle
to its own file as it's now being used in two actionssaveEditedEntityRecord
in the "Set as homepage" and "Set as posts page" actions so it's more consistent with other similar use casesWhy?
Addresses part of the second task in this comment.
This means that more of the homepage display controls from Settings -> Reading are replicated within the Editor, which hopefully makes changing them easier from within the Editor.
How?
By adding one new action, "Set as posts page".
Testing Instructions
Screenshots or screencast
Both "set as" actions:
Posts page confirm modal: