jetpack
jetpack copied to clipboard
Top Posts & Pages Block: Prevent Disabling all Types
Partly addresses #36109
Proposed changes:
- Prevents the user from disabling all content types under the "Items to display" section of the Top Posts & Page block.
Jetpack product discussion
See #36109.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
- Add a Top Posts and Pages block in the editor
- Try to disable all types
- Confirm that you see an error message when you do so upon trying to disable the final type
Thank you for your PR!
When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
- :white_check_mark: Include a description of your PR changes.
- :white_check_mark: Add testing instructions.
- :white_check_mark: Specify whether this PR includes any changes to data or privacy.
- :white_check_mark: Add changelog entries to affected projects
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:
The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.
Jetpack plugin:
The Jetpack plugin has different release cadences depending on the platform:
- WordPress.com Simple releases happen daily.
- WoA releases happen weekly.
- Releases to self-hosted sites happen monthly. The next release is scheduled for July 2, 2024 (scheduled code freeze on July 1, 2024).
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.
I’ll defer to Design folks, but my personal view is that this isn’t as significant an error to justify such a prominent position. We’re more trying to convey “you can’t do that!” as opposed to “this is an error!”, whereas I think notices within the editor’s content usually signals the latter.
Hey :)
The current placement works well since it contextualizes where the error occurred and how to fix it. Ensuring users understand what went wrong and how to resolve it is crucial, and this addresses that concern.
Regarding the wording, it might be beneficial to streamline it. For instance, the header "Items to display" and each label containing the word "Display" could be redundant. Have you considered removing the "Display" part and simply using "Posts; Pages; Media"?
Thanks @ilonagl!
Regarding the wording, it might be beneficial to streamline it. For instance, the header "Items to display" and each label containing the word "Display" could be redundant. Have you considered removing the "Display" part and simply using "Posts; Pages; Media"?
I feel that's probably out-of-scope of this PR, but I'm not sure that the "Item to display" header is redundant or that it would be beneficial to merge this section. For context, it offers some quite distinctive controls relative to the other settings.
Could deffo remove the "Display" from the "Display posts" though if that would be preferable.
@Aurorum
Could deffo remove the "Display" from the "Display posts" though if that would be preferable
Yes, that's the only place I had in mind that might benefit from being simpler. The rest is good as it is; as you mentioned, it serves different purposes.
Hey @Aurorum! Just doing some repo maintenance here: do you need a new review here?
Hey @monsieur-z - yes, that would be great, thanks! I seem to get changelog errors when trying to update with trunk though (I think this branch is too old), so would be great if you could help me rebase.
[1;31mProject packages/scheduled-updates is being changed, but no change file in projects/packages/scheduled-updates/changelog/ is touched![0m
I suggest you try the steps mentioned in this guide. Let us know how it goes!
@monsieur-z I rebased this branch. Can you give it a review?