-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Block visibility: render blocks when hidden at all viewports (and other changes) #74679
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
Block visibility: render blocks when hidden at all viewports (and other changes) #74679
Conversation
…nsistency and update test case for two breakpoints. Removed redundant check for hidden blocks on all breakpoints.
…ts" to "viewport sizes" for clarity, and adjust related test cases accordingly. This enhances consistency in the codebase.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| // translators: %s: The shortcut key to access the List View. | ||
| __( | ||
| 'Block visibility settings saved. You can access them via the List View (%s).' | ||
| 'Block visibility settings updated. You can access them via the List View (%s).' |
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.
Slipped this one in here 😄
Was mentioned that "saved" isn't quite right since nothing is saved until the post is saved.
|
Size Change: -1 B (0%) Total Size: 3.09 MB
ℹ️ View Unchanged
|
tellthemachines
left a comment
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.
LGTM! Feel free to ignore my comment about the breakpoint size, it doesn't really matter. I don't love that we need that final unused breakpoint but haven't any better ideas for now 😄
Thanks for adding the clarifying comment anyway!
| * Note: the last item in the $viewport_sizes array does not technically require a size, | ||
| * as the last item's media query is calculated using `width > previous size`. | ||
| * It's included for consistency and as a record of the "official" breakpoint size. | ||
| */ |
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.
Very minor point given this isn't used, but I wonder if it would be better to use $break-xlarge or even $break-wide value here because 960 is tiny as a max size for desktop.
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.
That makes sense to me. Thanks for double checking it.
Looking at https://github.com/WordPress/gutenberg/blob/trunk/packages/base-styles/_breakpoints.scss#L10 and elsewhere, I think I got this label + value relationship completely wrong.
The good news is this is the only place I've made that assumption so we can rename or delete it 😄 I might keep the comment there and remove the size.
What?
Related to:
This PR:
$viewport_sizeslast item to indicate that the final size isn't necessary technically (for now).Why?
Testing Instructions
There is only only one logic change on the frontend. The rest is formatting and renaming.
To test manually, enable the experiment:
In the editor, hide a block at all breakpoints.
Save the post and preview the frontend.
Inspect the hidden block, It should still be in the DOM but hidden by the
wp-block-hidden-desktop wp-block-hidden-mobile wp-block-hidden-tabletclasses.Tests should pass.