-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Block visibility: add visibility notice for hidden blocks in the block inspector #74180
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
|
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. |
| const parentHidden = parents.some( ( parentId ) => | ||
| _isBlockHidden( parentId ) | ||
| ); |
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 expect checking parents' visibility might be needed elsewhere as things progress. Just noting that it might be part of some hook or new selector down the track.
|
Size Change: +6.19 kB (+0.24%) Total Size: 2.6 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in cdee423. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20472865712
|
| const { getBlockParents } = select( blockEditorStore ); | ||
| const { isBlockHidden: _isBlockHidden } = unlock( | ||
| select( blockEditorStore ) | ||
| ); |
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.
Note: You can use a single store getter for both private and public selectors.
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 @Mamaduka! I'll update
| <HStack spacing={ 2 } justify="start"> | ||
| <Icon icon={ unseen } /> | ||
| <Text> | ||
| { hasHiddenParent |
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.
What should happen if both the current block is hidden and the parent is hidden? Currently the parent text overrides the child text.
The example is if you have a hidden block within a parent block that is also hidden.
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.
Good question. TBH I hadn't thought of that. I'll look into it. Maybe it should just default to "Block hidden" in that case.
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 it's something that's less likely to happen right now, but more likely when we get to the hiding-by-breakpoints behaviour. I.e. I could imagine someone might have a wrapper for "mobile and tablet" (hidden on desktop) and within it, they might have something hidden on mobile, or something hidden on 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 think this POC kinda tackled this scenario with variable messages (if I'm understanding you right)
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.
In general I like this idea of making it more visible that a block is hidden, especially in the block inspector.
Other than the store selector that George mentioned, the code looks good! Just left a comment re: whether we should use the parent hidden text or the block hidden text when a block is both hidden and has a parent hidden, but otherwise working nicely:
2025-12-23.16.30.37.mp4
The only other thing I'd check before landing is whether designers are happy with where the notice is positioned? But otherwise LGTM 👍
t-hamano
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.
Thanks for the PR!
I think the BlockCard component is an abstract component that simply displays information based on given props, so it seems strange to have block support logic inside it.
What if we added a new component like BlockVisibilityInfo here, between the Block Card component and the Edit section component?
<BlockCard
{ ...blockInformation }
allowParentNavigation
className={ isBlockSynced && 'is-synced' }
isChild={ hasParentChildBlockCards }
clientId={ renderedBlockClientId }
/>
<BlockVisibilityInfo clientId={ renderedBlockClientId } />
{ window?.__experimentalContentOnlyPatternInsertion && (
<EditContents clientId={ renderedBlockClientId } />
) }
I think the same applies to the navigation logic in the Block Card component. Especially, not that it'll be enabled for more blocks (#74164). |
Thanks for the idea. Sounds like a plan. |
|
I hear the motivation, but to me it's not clear it solves the underlying issue. The list view is not open by default, and if there are blocks hidden in the canvas, you won't see this notice until you find it and select it (arrow-key through the whole stack). And in the inspector, it looks like an error, when a hidden block will always at some point have been intentional. Even then you might have the inspector closed. I share the motivation to solve this, my current best idea is this one. |
I hear you, thanks for being on top of this feature 🙇🏻 Here, the only motivation was to test some of the ideas in the POCs, specifically #73888. What it looks like aside, I think we need to answer the question: does a message, or some sort of indicator like this around the block card provide any value at all? I'm not sure it matters that it's potentially hard to find, because users will find it. Rather, taking the inspector on its own, does it help and provide valuable info about the currently selected block or is it useless? If the latter, then let's just can it, agree. I don't feel strongly either way. It could be that it's slightly above neutral or just needs a different presentation. I don't think it conflicts with the floating/toolbar indicator per se. Another thing to keep in mind is that we can hide anything behind the current experiment.
Is there a middle ground here? I'm not familiar with the Notes feature (does it exist, or would it be bespoke to this?), so maybe my concerns might be unfounded, but my first worry is adding a larger dependency, especially one that has not yet been built. |
It might need some input by folks currently enjoying deserved AFK, so something to come back to, and yes there are always alternatives we can explore. This to me just feels the most compelling one, because:
One immediate alternative that comes to mind, if all else fails, is to actually show a notice, just like we do when there's revision history issues. That is, at the top of the canvas, with a cross to toggle it off: "There are hidden blocks, you can see them in the list view". I'm not necessarily recommending that quite yet, as noted I think we should explore the notes thing first, but it solves some of the issues an inspector notice does not, in that it is immediatly visible, actionable, and works with or without inspectors/list-view panels on. |
Ahhh, that Notes 😆 Sorry, it just clicked. Thanks for being patient with me.
Yeah, I do see the potential if we can piggy back off the system. Grouping a lot of hidden blocks on the same latitude might be finicky, but I've see you pull off harder miracles 😉 I'd feel better about 7.0 if we got the fundamentals of applying break points working first, and then dealt with discovery. Let's see how we get on. Cheers! |
SirLouen
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.
I had a little existential crisis with this conditional with booleans dancing in my head. 🙀
Maybe it could be clearer (as we are early returning before, both cannot be false).
For the rest, looking good ✅
packages/block-editor/src/components/block-visibility/block-visibility-info.js
Outdated
Show resolved
Hide resolved
|
It's still unclear to me this is the right solution. |
Good question. Should it? What do you think? My rationale was that a child block whose parent is hidden is also hidden, but only by virtue of its parent (ancestors included) not explicitly.
No worries. It's okay to be unsure. I won't merge it yet, or, better I'll put it behind the experiment before merging. As mentioned, this PR exists because @mtias proposed it in #73888, but also in the spirit of pragmatism and trying things out. I could be wrong, but I thought the stakes would be pretty low on this one. Happy to leave this PR particularly so because, for Add option to hide blocks based on screen size #72502 project to make progress in light of 7.0, I think we should build out features that allow folks to use the feature as soon as possible. For me this looks like, in order of priority:
Beyond those, I believe it's imperative to confront and unambiguously answer the following questions:
Cheers! 🍺 |
|
FWIW there was a thing that the experimental form block was using for the submission notices, like a diagonal striped pattern for non-visible blocks. Maybe this can feel crude for the design masters, but it's clearer than water, and I liked it a lot. Most builders use a pattern like this (or translucent effects). Generally I tend to like everything, unless it's too scrambled. |
|
Appreciate that added context, and there's a reason I've only ever left commments on this, it's in the category of "it's fine to land and try out". The motivation for adding notices to the inspector resonates for me when it relates to responsively hidden blocks, mainly because you could select one on the desktop breakpoint, preview mobile where it might be hidden, and thus the inspector notice provides an important cue. I get it. The main reason for being unsure relates it to generally hidden blocks, where you won't see the block on either breakpoint. In this case, it can't be a replacement for something like note-based indicators. Oh, and the Notice component feels very tired at this point, I do wonder whether semantically we could use Badge instead? Edit: Oh, and I don't think yo unecessarily need to add an experiment just for this notice. |
Very good point! I think responsively-hidden blocks was the main use case, I just thought I'd get a head start.
Looks okay to me!
|
|
A PR like this we can afford to keep on ice for a bit until the foundations are laid. Thanks for the discussion and everyone's input 🙇🏻 |
Thanks @t-hamano! I need to rebase this PR. Will do that. |
Co-authored-by: Manuel Camargo <[email protected]>
1909653 to
aab1505
Compare
Done! Looks like the block card was updated independently - I guess after #74164?
|




What?
Related to:
#73888 proposed a block visibility notice in the block inspector
This PR implements the "hidden everywhere" aspect with the intention to extend the notice to display viewports later.
This will be a series of little PRs to implement #72502
Why?
For discoverability mainly. Hidden blocks are not displayed on the canvas, though they can be selected.
The notice informs the user that a block is hidden.
How?
By checking if a block, or one of its parents, is hidden.
Testing Instructions
Example block HTML
Screenshots or screencast