Block Bindings: Allow only admin users to create and modify bindings by default #64570
Conversation
SantosGuillamot
added
[Type] Enhancement
|
Size Change: -858 B (-0.05%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d357926. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10420898477
|
|
Always good to press the issue. I have the experiment enabled, at the moment, and I think there are a couple of other things we can try first. For further context, when I have no bindings registered, and no plugins active, I don't see the panel:
That's great, it's how it should be. As soon as I enable Jetpack, it shows up, like this:
This is because Jetpack registers a 
Changes here:
Both of these would reduce the footprint of the panel when you're not using it, to the point that it might be fine to have there, without explicitly enabling or disabling it. Which is also to say, we might still add an option to disable it, though perhaps it should default to on? |
|
Joen's proposal makes plenty of sense to me. I'd go one step further and don't display the help text at all, no matter if there are attributes or not. We don't explain what other sections are for so this feels a bit inconsistent. Also, the text itself isn't very helpful because it doesn't say what attributes or sources are, and given that it's only shown if there's at least one connection, the user is likely familiar with the concept. |
|
Figma, btw, if you want to tinker! Forgot to add that in my previous comment. |
|
Yes, the empty panel is something we wanted to improve as well. And the suggestions made make sense to me. Thanks for sharing them 🙂 On the other hand, I believe we can work on that on another pull request.
If that's the case that might be a "bug". When you have the experiment enabled, the idea is to always show the empty panel because users should be able to create bindings for an empty paragraph, for example.
The main problem with having the default to on is the paragraph block. Right now, the paragraph block doesn't have any "Settings", so when you enter a new paragraph, it just shows the "Styles". If we introduce a new setting like in this case, it creates a new tab for "Settings", which is selected by default. That means that it changes the workflows of any paragraph. You can find more context in this comment in the UI concerns section: link.
I don't like the current empty panel and I agree that "Attributes connected to various sources" sounds confusing. On the other hand, I feel just having "Attributes" isn't clear enough for users. We discussed in the past having Bindings or Connections but the feedback was it is too technical. Other possibilities could be "Dynamic Data" or "Custom Fields" if we decide to keep this UI just for custom fields and not other sources. |
Right, good point.
That does seem to be the key issue to solve indeed. Can we make an exception for this panel, just like there is an exception for "Advanced", so that it doesn't split the inspector? It would still, like the Advanced panel, show up only on the Settings tab if that tab is present. |
That's something we considered and I believe I actually have a local branch with that change working. However, it would mean having this panel in the "Styles" tab, which doesn't seem correct and it is some feedback we got if I remember correctly. |
Are you sure? The "Advanced" panel doesn't show up in the Styles tab, only in the Settings tab, yet it doesn't cause the Paragraph to have the split tabs. |
|
I might be confusing what is "Styles" tab and what is "Settings" tab. For the paragraph this is how it looks like:
I assumed those were just styles and that's why I called it "Styles" tab. Although it's true there is no mention for "Styles". I'm happy to introduce it above the "Advanced" section. |
|
Ah, in that case, the "Advanced" panel is exempt from the rules. For any inspector that splits in Settings & Styles, the "Advanced" panel will show up on the "Settings" tab. But it doesn't, as you can see, cause the Paragraph to split into those two. And similarly I could see the same behavior applied to Attributes. |
|
I have just pushed some changes to make it work that way:
I agree that we need to revisit the help text and probably remove it when there are no bindings. I still believe "Attributes" is not clear enough. What would you think about this:
|
Yes.
I quite like "Attributes", I think we should keep it for the time being.
I'm personally fine with help text being present when there are bindings, and I'm flexible on the verbiage. Though I do like that the current help text fits on one line. If we do have to go to two lines, need to make sure it doesn't wrap in a way that creates a typographic widow. That should be automatically handled due to |
SantosGuillamot
changed the title
|
I have just pushed those changes to the help text. It doesn't appear when there are no bindings
It shows when there are bindings with a different text
|
|
I believe this is ready for review. Now it adds a way to enable/disable this UI and it fixes the issue of interfering with the paragraph block. I guess there are two questions:
In my opinion, it is fine for this first iteration and we can keep working on top of it. I'm gonna be on holiday next week, feel free to push any required changes. |
SantosGuillamot
requested review from
spacedmonkey,
tellthemachines and
ellatrix
as code owners
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jarekmorawski. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
Should post meta that starts with an underscore be exposed by the Block Bindings API? It's a precedent that fields starting with an underscore are "hidden" and don't show up in the regular Custom Fields UI. Feels like they shouldn't show up in Bindings either. https://developer.wordpress.org/reference/functions/add_post_meta/#comment-467 |
Right now, it shows only the fields that are explicitly marked as public to be exposed in the REST API. I assumed that was enough, but I'm happy to hide the ones starting with underscore if that's expected. I don't have enough context to say if that's the case. |
|
Yes, that makes sense to me as well. If we add that setting, do we keep the preference or do you consider it is not needed anymore? |
|
The preference can be added later if necessary. One thing to keep in mind if we go with the editor preference, then if it gets set on the server to I was surprised to learn that gutenberg/packages/block-editor/src/store/defaults.js Lines 157 to 158 in 8236cf6 |
|
Okay, trying to summarize what this means:
Does this make sense? |
Yep, still I don't know what to say about editor or authors with their own content. Let's explore later. |
|
I've made the mentioned changes in the latest commits. With that, this pull request covers:
I'll work on a backport in core, and I'll update the description to reflect these changes, but everything should be working now. |
SantosGuillamot
changed the title
There was a problem hiding this comment.
Tested this and it works as expected based on the most recent expected behavior (though the PR description as of this writing still needs to be updated) — admins can edit the attributes, editors cannot.
I do think we should add additional styling for the non-admin read only state, like setting the attribute name to gray, to signal that the the UI there has been disabled. Otherwise, the fact that it's disabled could be interpreted as a mistake.
What do you think?
| function gutenberg_add_can_update_block_bindings_editor_setting( $editor_settings ) { | ||
| if ( empty( $editor_settings['canUpdateBlockBindings'] ) ) { | ||
| $editor_settings['canUpdateBlockBindings'] = current_user_can( 'manage_options' ); | ||
| } |
There was a problem hiding this comment.
I'm noting that a discussion around how these permissions should be set up is happening in the core PR: https://github.com/WordPress/wordpress-develop/pull/7258/files#r1736575839
There was a problem hiding this comment.
It should be fine to start with, but we need to sort it out and sync back to Gutenberg when we arrive on the final way for handling it in WP core. Thank you for connecting the dots 👍🏻
This part from the #64570 (comment) left by @jasmussen wasn't addressed. Have you decided to keep the message? In my opinion, it doesn't bring too much value when no connections are established. |
I agree the "read-only" panel could be probably improved. On the other hand, it isn't still clear to me how. I'd vote for creating a separate discussion for that and make the improvements in a follow-up PR. In the end, the "read-only" panel hasn't changed in this PR, the styling is the same that we have right now in
Mmm, it is supposed to be addressed. I shared how it was working in this comment. I have just checked the branch and it is working as expected for me.
Did you see anything different or should I just update the description? |
The description and screenshot in the comments are misleading. I couldn't test it using Playground for some reason for the case with no connections. I don't have any more comments then 👍🏻 |
|
I have just updated the screenshots in the description. I hope that helps clarifying the current status. |
There was a problem hiding this comment.
I agree the "read-only" panel could be probably improved. On the other hand, it isn't still clear to me how. I'd vote for creating a separate discussion for that and make the improvements in a follow-up PR. In the end, the "read-only" panel hasn't changed in this PR, the styling is the same that we have right now in trunk without the experiment enabled.
That works for me. Let's definitely have that discussion and aim to get those styling changes in for 6.7.
No additional comments from me either 👍
fabiankaegy
added
the
Needs Dev Note
Just wanted to say how much we appreciate this inclusion. 🙏 (We run MultiSites and our Site Administrators should absolutely not be able to do this kind of stuff. The ability for us to remove that capability from folks with a server-side filter is everything! Cheers!) |
What?
This pull request removes the experimental flag to enable the block bindings UI to create connections and tries an approach where only admin users can create and modify bindings by default.
As part of it, it includes a new
canUpdateBlockBindingseditor setting to let developers override this behavior.It creates a "bindings" group in the inspector controls that follows the same "workflows" as the Advanced panel. This means that in the blocks where there are no "Settings", it shows in the initial list. This solves the issue where every empty paragraph was adding the panel and changing the workflow.
Additionally, it address some improvements to the UX, like not showing the help text when there are no attributes connected.
Why?
It can remove the experiment while keeping it safe only for admin users and provides a method to change this behavior. And it solves the issue with empty paragraphs.
How?
I added a new
canUpdateBlockBindingseditor setting that it is true for admin users by default. This setting is read from the block bindings component to decide if it should show as "read-only" or it should allow the user to create and modify connections.Apart from that, I created a new "bindings" group to put the panel just above the Advanced section in the different scenarios.
Testing Instructions
Logged as an admin
Logged as an editor