Data Views: Add action for pages to set site homepage #65426
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
creativecoder
added
[Feature] Site Editor
|
I thought it was interesting how you handled non-published pages but I noticed an issue; if you set a scheduled page as the homepage it works, but the page remains scheduled resulting in a 404 on the frontend. It might make sense to restrict the action to
The Snackbar copy could use some refinement. Maybe just "Homepage updated"? Testing the flow with reading settings set to display latest posts caused the editor to crash: 
Relating to this, I noticed an oversight in the original design; when setting a static homepage and offering the user the option to set a 
|
creativecoder
force-pushed
the
try/site-editor-homepage-settings
branch
from
ab1c828
to
0f3a986
Compare
|
Size Change: +734 B (+0.04%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
@creativecoder is this one ready for review? :) |
|
@jameskoster A few things that still need doing
I'm not sure when I'll be able to get back to this, so if anyone wants to pick this up--feel free! |
|
Thanks @creativecoder! I'm going to have a go at picking this up. I'll start by addressing the items you listed above, and then I'll mark as ready for review. |
getdave
reviewed
Nov 20, 2024
| }; | ||
|
|
||
| return ( | ||
| <form onSubmit={ onSetPageAsHomepage }> |
There was a problem hiding this comment.
With a slow network the UX doesn't provide a great experience.
There should be a selector/resolver which we can use to detecting that the saving of the entity is in progress. We can then use that state to render a "Loading..." type indicator for the user.
There was a problem hiding this comment.
Yes, nice idea! I've added the isSavingEntityRecord selector and disabled the modal button when saving is in progress: b029794.
There was a problem hiding this comment.
It's a good improvement thanks! However I don't think it's enough because in onSetPageAsHomepage we are also doing other "work" such as updating the page status.
As a result the buttons are still enabled whilst that request is happening. I think we'll need to have the resolvers for any action that's occurring in the onSetPageAsHomepage handler.
See screencapture.
Screen.Capture.on.2024-11-22.at.09-46-58.mp4
There was a problem hiding this comment.
I think this is not longer relevant now that we don't allow for draft Pages to be set as the homepage?
There was a problem hiding this comment.
I think this is not longer relevant now that we don't allow for draft Pages to be set as the homepage?
Yes that's right, for now, but it will likely need to be handled in the follow-up that handles draft pages.
| type: 'snackbar', | ||
| } ); | ||
|
|
||
| onActionPerformed?.( items ); |
There was a problem hiding this comment.
We should stick a debugger here and follow the code route. I suspect we don't need to call this in this scenario.
There was a problem hiding this comment.
@oandregal Is there any documentation about onActionPerformed? I looked and couldn't find any.
There was a problem hiding this comment.
I've removed onActionPerformed for now: 04a7ba4.
getdave
reviewed
Nov 20, 2024
Co-authored-by: Dave Smith <getdavemail@gmail.com>
6 tasks
|
Thanks again for all the continued feedback and help here ❤️ I'll summarise the latest changes since @oandregal's approval:
I'm really happy with the current state, so I'm hoping this is now ready for a final review 🤞 Thank you! |
getdave
reviewed
Nov 22, 2024
| }; | ||
|
|
||
| return ( | ||
| <form onSubmit={ onSetPageAsHomepage }> |
There was a problem hiding this comment.
It's a good improvement thanks! However I don't think it's enough because in onSetPageAsHomepage we are also doing other "work" such as updating the page status.
As a result the buttons are still enabled whilst that request is happening. I think we'll need to have the resolvers for any action that's occurring in the onSetPageAsHomepage handler.
See screencapture.
Screen.Capture.on.2024-11-22.at.09-46-58.mp4
# Conflicts: # packages/editor/src/components/post-actions/actions.js
|
@getdave Thanks for such a detailed review 🙏 I think I've addressed all your feedback, with the larger changes being:
|
|
👍 I'll re-review this. |
getdave
approved these changes
Nov 27, 2024
There was a problem hiding this comment.
Overall I think this is in good shape.
Left a few nits both nothing major.
Tested with slow network and all responding really well.
One thing I noticed is that it's not possible to "unset" the Page as the homepage. Not sure if that's planned or necessary but thought it might be worth mentioning for a followup.
Thanks for all the work to refine this 👏
Comment on lines
85
to
95
| <> | ||
| <Text> | ||
| { sprintf( | ||
| // translators: %s: title of the page to be set as the homepage. | ||
| __( | ||
| 'Set "%s" as the site homepage? This will replace the current homepage which is set to display latest posts.' | ||
| ), | ||
| pageTitle | ||
| ) } | ||
| </Text> | ||
| </> |
There was a problem hiding this comment.
Nit: Why does this need a fragment with <>?
I keep coming to this block and thinking there must be some fundamental difference in the structure of the JSX but there isn't.
Therefore do you think we should normalise the component returned and just have a variable for modalText passed in (you can precompute this outside of the JSX).
Example:
<Text>
{ modalText }
</Text>
There was a problem hiding this comment.
I agree, I can't see why there was a fragment there and this string can be refactored into a variable. I've done this here: e1a7aa9.
| }; | ||
|
|
||
| return ( | ||
| <form onSubmit={ onSetPageAsHomepage }> |
There was a problem hiding this comment.
I think this is not longer relevant now that we don't allow for draft Pages to be set as the homepage?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds an action to set the home page from the site editor.
Fixes #63666
Why?
Currently there is no way to change the home setting (Settings > Reading) from the site editor. This PR adds a way to do that.
How?
Adds a dataview action for pages to set the page as the site homepage that can be triggered from the pages data view.
Testing Instructions
Screenshots or screencast