jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

Top Posts & Pages Block: Prevent Disabling all Types

Open Aurorum opened this issue 1 year ago • 9 comments

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
Screenshot 2024-03-09 at 10 32 36

Aurorum avatar Mar 09 '24 10:03 Aurorum

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.

github-actions[bot] avatar Mar 09 '24 10:03 github-actions[bot]

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.

Aurorum avatar Mar 18 '24 14:03 Aurorum

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"?

ilonagl avatar Mar 19 '24 09:03 ilonagl

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.

Screenshot 2024-03-19 at 20 19 04

Could deffo remove the "Display" from the "Display posts" though if that would be preferable.

Aurorum avatar Mar 19 '24 20:03 Aurorum

@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.

ilonagl avatar Mar 20 '24 10:03 ilonagl

Hey @Aurorum! Just doing some repo maintenance here: do you need a new review here?

monsieur-z avatar May 07 '24 15:05 monsieur-z

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

Aurorum avatar May 10 '24 10:05 Aurorum

I suggest you try the steps mentioned in this guide. Let us know how it goes!

monsieur-z avatar May 10 '24 13:05 monsieur-z

@monsieur-z I rebased this branch. Can you give it a review?

kraftbj avatar May 15 '24 15:05 kraftbj