-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Block visibility based on screen size: add rules to hide on viewport size #74379
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
Conversation
…if a block is hidden.
|
Size Change: +499 B (+0.02%) Total Size: 3.08 MB
ℹ️ View Unchanged
|
packages/block-editor/src/components/block-visibility/use-block-visibility.js
Show resolved
Hide resolved
| }, [ deviceType, isLargerThanMobile, isLargerThanTablet ] ); | ||
|
|
||
| // Determine if block is currently hidden | ||
| const isBlockCurrentlyHidden = useMemo( () => { |
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.
I did consider adding classnames, but the effect would be the same.
The current method, also in trunk, is that the block isn't rendered to the block tree at all, so I was thinking we just need a flag for now.
There's another conversation on whether we should be rendering/hiding via CSS, and exactly how. It was the motivation behind this experiment:
|
I just did a quick test on playground and this is working well except that, on resizing the window, the blocks flagged as hidden in the list view don't change: list-view-resize.mp4They only change when using the preview device sizes. |
…n size detection and update changelog with additional Gutenberg PR link.
Thanks for testing!!! Yes, that's expected. This PR is to create the underlying state that will inform the UI. The UI I've separated into another PR That one is getting big too, so there might be several "UI" PRs. Anyway, if this PR gets in, I'll update over there. |
|
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. |
andrewserong
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.
This is generally testing well for me, too. The main thing I noticed is that the tablet breakpoint is different in the editor vs on the site frontend. This very well is likely true in trunk, too, as that's what the Tablet dropdown maps to.
For this PR, I reckon we should pick one and make sure the front and backends match.
Other than that, I noticed the test file seems a bit verbose with lots of mocks which made them a little hard to read for me. Not a blocker, but is it worth simplifying / consolidating the tests? The file they're testing is quite small, which is the main reason I mention it, but this is a bit nitpicky. Overall, glad to see good test coverage for these things 👍
Besides the tablet breakpoint mismatch, this looks like a good iterative step to me!
| // When Mobile/Tablet is selected, override with device type | ||
| // All hooks must be called unconditionally | ||
| const isLargerThanMobile = useViewportMatch( 'mobile', '>=' ); // >= 480px | ||
| const isLargerThanTablet = useViewportMatch( 'medium', '>=' ); // >= 782px |
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.
This uses 782px in the editor vs the site frontend which uses 960px. Should this use large instead, or is it intended that we'll move to a narrower tablet breakpoint?
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.
Oh wow, good spotting. Another boo boo.
The frontend is wrong there. It should follow the medium size for tablet
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.
I've updated here and on the backport.
I expect we might be revisiting the frontend anyway.
Thanks!
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.
Cheers, I'll take this for another spin this morning 👍
…S and corresponding unit tests.
Thanks for testing @andrewserong! What did you have in mind? The mocking is for the useViewportMatch/useSelect. I think, to make it easier to test we could isolate the logic into utils outside the hook? |
Thanks, I was going to propose something but I ran out of time yesterday when Claude got stuck refactoring them 😆 I think isolating the logic into utils outside the hook might be the neatest approach since that's the bit we want to test anyway. I don't really mind verbose tests, the main challenge I have is that when test files are really long I'm prone to not reading them properly when reviewing and just trusting that if they pass they're good. I'm sure this is fine most of the time, but there's a slim chance we might have passing tests that aren't really covering things properly. Again, I'm probably being a bit nitpicky here! 😄 |
Not at all. If the tests are hard to read, in my opinion, that does hide code smell. Personally, I rely on the tests to understand what the code is doing more often than looking at the code itself. I'll try to split out the logic. Thanks! |
|
I replied to your comment before I looked at your code change! I reckon e1b23c6 already tidies things up nicely, no need to split things out further if you haven't started on it yet 👍 |
Cheers! I started looking and ended up abstracting things too far and returning to where I started 😄 I'll leave it then for now. |
andrewserong
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! We'll of course continue tweaking all this stuff in follow-ups, but this is looking good to me 👍
…related components after rebase on #74379
| // component, and useBlockProps. | ||
| function BlockListBlockProvider( props ) { | ||
| const { clientId, rootClientId } = props; | ||
| const { isBlockCurrentlyHidden } = useBlockVisibility( clientId ); |
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.
I think this line may have caused a performance regression - https://www.codevitals.run/project/gutenberg/inserterHover
Probably because of the extra store subscription that the hook adds. It might be possibly to resolve it by using the existing useSelect.
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.
Oof! Thanks for catching that. I'll get a PR up. If it doesn't resolve, I'll revert and find another way
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.
Attempt 1: #74481
Part of:
What?
Adds
useBlockVisibilityhook to determine if blocks should be hidden based on viewport/device settings.Follow up to
Note
This PR just provides the "state" flag of whether a block is hidden or note. The UI based on that will be in #74249
Why?
This harmonizes the editor with the frontend experience.
How?
metadata.blockVisibilityobject wherefalse= hidden{ isBlockCurrentlyHidden, currentViewport }Testing Instructions
Some test HTML
Screenshots or screencast
Kapture.2026-01-06.at.17.20.24.mp4