Add Block Bindings Panel to Block Inspector #61527
Conversation
|
Size Change: +1.15 kB (+0.07%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Here is a first pass at adding a Block Bindings Panel. I aimed mostly to just get it working and am unsure of best practices regarding passing around block instances vs. introducing additional selectors inside of new components. I figured I would open this to start getting some ideas and feedback before refactoring. Any thoughts appreciated!
packages/block-editor/src/components/inspector-controls-tabs/settings-tab.js
Outdated
Show resolved
Hide resolved
|
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. |
artemiomorales
added
[Feature] Block bindings
[Type] Enhancement
|
@jasmussen @richtabor I pinged you so you can see a live version of the Block Bindings Panel and see how it feels. I'm thinking to remove prior UI from the canvas in a separate PR to keep the code easy to review. |
artemiomorales
changed the title
|
I didn't take a deep look at the code yet, but I am not sure we should modify the existing components for the inspector just to include the bindings and also exposing a new component that won't probably be reused outside of this use case. Can't we use a |
Yeah that makes sense! I'll take a look at it more closely soon, and would also like to give space for thoughts from other folks. |
|
Nice, progress!
Just at a quick glance, this panel feels like a great way to add clarity to connected blocks. As soon as you select one, the panel appearing in the inspector makes it very clear this is a connected block. Unrelated to this, we need to make progress on the ItemGroup items, they are 42px tall and should be 40px. @mirka do you know if there might be an easy fix here? |
|
@SantosGuillamot I've refactored this PR and put the Block Bindings Panel inside of the |
| .components-panel__block-bindings-panel { | ||
|
|
||
| .components-item { | ||
| display: flex; | ||
| justify-content: space-between; | ||
|
|
||
| .components-item__block-bindings-source { | ||
| color: $gray-700; | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
How are these styles different from the original components? It'd be great to analyze and discuss if this is something specific to this use case or it is something other cases would benefit.
There was a problem hiding this comment.
@jasmussen Not sure if you'd be able to offer insight here — should we abstract this style so that it's reusable for other features?
To me, this styling here to create a left and right column within the Item group does seem pretty specific to the block bindings panel.
There was a problem hiding this comment.
I don't know if it is common to other components, but I was thinking of adding properties to handle different use cases.
For example, I can see other components have a color property. Does it make sense for this component? Why?
Addtionally, the structure looks really similar to the fonts panel showed here. Or even for other things like shadows.
There was a problem hiding this comment.
Not sure if you'd be able to offer insight here — should we abstract this style so that it's reusable for other features?
Almost certainly, yes. @madhusudhand is working on ItemGroup enhancements as well: #60706 (comment)
Whether we can get the ItemGroup component updated in time for this panel to land in 6.6, or you have to use a local temporary setup, that's actually less important — we can componentize and systematize at a later time.
We know this:
- ItemGroup is the right component for this panel, and for shadows, and for fonts.
- We want the ItemGroup component itself to optionally support a "minus" to remove/clear. We need that for fonts, even, for quick removal.
- An optional help-text or "tip" column on the right, feels like a valid prop to have as well. It might be mutually exclusive with some other items, like the reset button. But here's a recent use case.
All of those cases may initially be separate things, but conceptually, they're all ItemGroups, and ideally we reach a space where the singular ItemGroup component can support those cases, without local CSS to support it.
There was a problem hiding this comment.
For example, I can see other components have a color property. Does it make sense for this component? Why?
@SantosGuillamot Which components are you looking at?
Addtionally, the structure looks really similar to the fonts panel showed #60706 (comment). Or even for other things like shadows.
The structure looks similar but in the case of fonts, they're actually buttons, whereas each item consists of two span in this PR at the moment. Once we add the functionality to remove bindings, these items will likely also become buttons instead and perhaps the discrepancy in structure will get resolved then.
I'm currently looking at the Shadows structure and see there are some things that may translate to the Bindings Panel — will give that a shot soon.
Whether we can get the ItemGroup component updated in time for this panel to land in 6.6, or you have to use a local temporary setup, that's actually less important — we can componentize and systematize at a later time.
Ok, what I'm hearing is to give consolidation a shot with what already exists, but if it doesn't yet, let's just get this merged then consolidate everything later. Let me know if that sounds incorrect!
There was a problem hiding this comment.
Can we land the proposed UI first and iterate on abstracting CSS later?
There was a problem hiding this comment.
@SantosGuillamot Which components are you looking at?
I just clicked something random, but it seems the Heading component has it. Or we could have variant/styles like in the button component.
An optional help-text or "tip" column on the right, feels like a valid prop to have as well.
I like the idea of having a prop like this.
So, if we include two new props to the component, would we still need the custom CSS? I'm thinking of:
- help-text or "tip".
- color/variant.
Is this something that could be implemented to be reused?
There was a problem hiding this comment.
Can we land the proposed UI first and iterate on abstracting CSS later?
This sounds good to me.
I like the idea of having a prop like this.
So, if we include two new props to the component, would we still need the custom CSS? I'm thinking of:
- help-text or "tip".
- color/variant.
Is this something that could be implemented to be reused?
Seems like they could probably be reused, and seems like this could be good to explore in a separate PR specifically related to the ItemGroup.
Addtionally, the structure looks really similar to the fonts panel showed #60706 (comment). Or even for other things like shadows.
@SantosGuillamot Taking inspiration from shadows, I ended up using the existing HStack component for the Bindings Panel rather than using custom CSS in one of the most recent commits 👍
|
I left some comments that are related to performance optimizations. In this case, it probably doesn't matter much, but those are good practices to follow and should be straightforward to apply. Nice work overall. 👍 Some e2e test fail. It might be two things. There is a new panel present in the sidebar so some existing tests might make assumptions about the order of panels. In the case of the multiselection tests, it might require showing this panel only when a single block is selected. |
artemiomorales
force-pushed
the
add/block-bindings-panel
branch
from
c56926d
to
f341e60
Compare
These heights are currently determined by the content height and paddings, rather than an absolute height value. Looks like we can get irregular heights in the block editor due to the cascading |
|
@jasmussen As revealed by the end to end tests, I realized that we should probably have an error state when users try to use a binding source that hasn't been registered. How does this look? 
|
| .label | ||
| ) : ( | ||
| <span className="error"> | ||
| Error: Unknown Source |
There was a problem hiding this comment.
This should be translatable.
What about the case when the source gets registered only on the server? It's all custom sources these days because the client-side API is still private. @SantosGuillamot and I were discussing exposing the registered sources from the server in a different PR
There was a problem hiding this comment.
What about the case when the source gets registered only on the server? It's all custom sources these days because the client-side API is still private. @SantosGuillamot and I were discussing exposing the registered sources from the server in a different PR
@gziolo I don't follow 🤔 Sources that get registered on the server using register_block_bindings_source() need to provide a label, which is what we would use here. The error would only arise when users try to bind a block to a source and mistype the source's name, or if they attempt to connect to a source that hasn't been registered. And as far as I see, the panel would get rendered regardless of what kind of block it is, core or custom, as long as it has the metadata.bindings property (though this should probably be tested — will take a look at that).
To clarify, what are you referring to when you say "client-side API", and how does it relate to this scenario?
There was a problem hiding this comment.
The sources we are consuming in the editor need to be registered in JS as well. They provide functions to know how to get the values and how to update them in the editor. One example could be the post-meta source that is registered here.
Registering in the client is not mandatory, and it doesn't mean it won't work on the server. So it is not exactly an error. I believe right now we are using "Dynamic Data" as a generic message, but we could also fallback to the key maybe.
In the future, as Greg mentions, we would like to reuse the server registration somehow, but that will be handled in a separate PR.
There was a problem hiding this comment.
Ok got it! Thanks so much for the explanation, that's very helpful 😄
I can revise to use the name in the binding when a label isn't present 👍
There was a problem hiding this comment.
Update: I've replaced the error with the machine-readable name.
This will likely very quickly get cramped. We can try and see if an error message can fit there, otherwise I'd be tempted to put an alert icon, and instead put a far more verbose error message in the actual flyout, which would fit translations. As far as text, use sentence case across. And I wonder, is "Unknown source" or "No source" sufficient? |
I like the "unknown" more. It could be also "unsupported" or "unrecognized". Should we also use some |
|
You all should decide, as you have context I don't. The main piece is to seek as short a term as possible, as it's a really constrained context. |
|
I agree that we should probably change the Error message. I tried to explain it here, but basically, it doesn't imply it is an error. I believe I would personally fall back to the name used in the bindings instead of the label if it is not defined. It would be something like "plugin/custom-source" instead of "Plugin Custom Source", for example. |
What?
Adds a panel that shows the current active block bindings to the Block Inspector.
Why?
Addresses #61404
We want to begin adding a UI for block bindings, which would make the feature easier to understand and use.
How?
It creates a new Block Bindings Panel component and includes it inside the Block Inspector based on whether the currently selected block has any bindings.
Testing Instructions
1. Register post meta by adding this snippet to your theme's functions.php
2. Add some bound blocks to a new post using the Code Editor
You may also create your own blocks and bindings if you prefer.
Testing Instructions for Keyboard
Screenshots or screencast