-
Notifications
You must be signed in to change notification settings - Fork 799
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
Top Posts & Pages Block: Prevent Disabling all Types #36305
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
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.
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 :)
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!
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. |
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
|
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? |
a33400f
to
b423d94
Compare
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.
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 👍
Partly addresses #36109
Proposed changes:
Jetpack product discussion
See #36109.
Does this pull request change what data or activity we track or use?
No.
Testing instructions: