Skip to content
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

Top Posts & Pages Block: Prevent Disabling all Types #36305

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

Aurorum
Copy link
Contributor

@Aurorum Aurorum commented Mar 9, 2024

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

@github-actions github-actions bot added [Block] Top Posts [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Mar 9, 2024
Copy link
Contributor

github-actions bot commented Mar 9, 2024

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ 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 🤖


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 github-actions bot added the OSS Citizen This Pull Request was opened by an Open Source contributor. label Mar 9, 2024
@anomiex anomiex added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Mar 11, 2024
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this makes sense, part of me wonders if i wouldn't be best to have a notice within the editor's content instead, where it would be more visible? What do you think?

Also cc'ing @Automattic/jetpack-design on this, they may know best :)

@Aurorum
Copy link
Contributor Author

Aurorum commented Mar 18, 2024

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.

@ilonagl
Copy link

ilonagl commented Mar 19, 2024

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

@Aurorum
Copy link
Contributor Author

Aurorum commented Mar 19, 2024

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.

@ilonagl
Copy link

ilonagl commented Mar 20, 2024

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

@monsieur-z
Copy link
Contributor

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

@Aurorum
Copy link
Contributor Author

Aurorum commented May 10, 2024

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

@monsieur-z
Copy link
Contributor

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

@kraftbj
Copy link
Contributor

kraftbj commented May 15, 2024

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

@kraftbj kraftbj requested a review from monsieur-z May 15, 2024 15:07
Copy link
Contributor

@coder-karen coder-karen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Aurorum for creating this PR!

Based on the review comments from Design and on reviewing the code (and testing), I'm giving this a 👍

@coder-karen coder-karen added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 13, 2024
@coder-karen coder-karen merged commit 20e27d1 into Automattic:trunk Jun 13, 2024
55 checks passed
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 13, 2024
@github-actions github-actions bot added this to the jetpack/13.6 milestone Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Top Posts OSS Citizen This Pull Request was opened by an Open Source contributor. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants