Block Bindings: Add warning when block attribute is connected to an invalid source #64893
Conversation
SantosGuillamot
added
[Type] Enhancement
|
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. |
| <Button | ||
| __next40pxDefaultSize={ false } | ||
| key="remove-all-bindings" | ||
| onClick={ () => | ||
| updateBlockBindings( { | ||
| [ invalidBinding.attribute ]: undefined, | ||
| } ) | ||
| } | ||
| variant="primary" | ||
| > | ||
| { __( 'Remove connection' ) } | ||
| </Button> |
There was a problem hiding this comment.
Do you think it is worth adding the button to remove the invalid binding?
There was a problem hiding this comment.
I should wait for this option for more feedback. This case should only happen on development sites, so maybe it does not worth the extra work.
Removing connections with the UI requires some knowledge about how tools component work. And I guess people will ask for a button to add or remove connections via block ToolsPanel (the same you use for putting the text bold).
There was a problem hiding this comment.
You're right that if we only allow admin users to create and modify bindings, it could be "risky" to add this button for all users. Maybe we can just remove it and add it if considered necessary in the future. What do you think?
There was a problem hiding this comment.
I should wait for this option for more feedback. This case should only happen on development sites, so maybe it does not worth the extra work.
I'm not sure this is correct. What about the case wherein a plugin adds a block bindings source, the source is used in a block, and the plugin is later uninstalled? I believe this warning would appear in those cases, though I could be wrong.
You're right that if we only allow admin users to create and modify bindings, it could be "risky" to add this button for all users. Maybe we can just remove it and add it if considered necessary in the future. What do you think?
I agree that allowing non-admin users to remove bindings could be risky. I also think that omitting the button may introduce inconsistencies in the user experience. Perhaps we should allow design to weigh in?
In any case, I think we should have some mechanism for fixing or removing the invalid binding. Notice how, when the connection is broken, we have no block settings — and, consequently, no attributes panel — in the sidebar:
Let's say you install a plugin with a custom binding source, make a few connections, but later decide you no longer want to use the plugin. How are admins supposed to not just be alerted to the broken bindings, but given a way to resolve them?
If we do remove the button for the non-admin users, we could give them a slightly different warning. Maybe something like, "Attribute "content" is connected to undefined "core/undefined" block bindings source. You have insufficient permissions to modify it. Please contact your site administrator."
There was a problem hiding this comment.
What about the case wherein a plugin adds a block bindings source, the source is used in a block, and the plugin is later uninstalled? I believe this warning would appear in those cases, though I could be wrong.
Yes, if a source is unregistered (by uninstalling the plugin), the warning will show. It works the same way with unregistered blocks.
I don't know if showing different messages for different users is something done in other places. Personally, I would lean towards:
- If the user can edit the post, show the button even if it is not an admin. I think removing broken connections is not that risky.
- Don't show the button until it is clearer how it should work.
SantosGuillamot
changed the title
|
Size Change: +458 B (+0.03%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
gziolo
requested review from
ndiego,
ryanwelcher,
justintadlock and
juanmaguitar
|
In this commit, I've removed all the e2e tests where we were checking that the appropriate controls where locked when connected to an Let me know if you think I should change it. |
| <Button | ||
| __next40pxDefaultSize={ false } | ||
| key="remove-all-bindings" | ||
| onClick={ () => | ||
| updateBlockBindings( { | ||
| [ invalidBinding.attribute ]: undefined, | ||
| } ) | ||
| } | ||
| variant="primary" | ||
| > | ||
| { __( 'Remove connection' ) } | ||
| </Button> |
There was a problem hiding this comment.
I should wait for this option for more feedback. This case should only happen on development sites, so maybe it does not worth the extra work.
I'm not sure this is correct. What about the case wherein a plugin adds a block bindings source, the source is used in a block, and the plugin is later uninstalled? I believe this warning would appear in those cases, though I could be wrong.
You're right that if we only allow admin users to create and modify bindings, it could be "risky" to add this button for all users. Maybe we can just remove it and add it if considered necessary in the future. What do you think?
I agree that allowing non-admin users to remove bindings could be risky. I also think that omitting the button may introduce inconsistencies in the user experience. Perhaps we should allow design to weigh in?
In any case, I think we should have some mechanism for fixing or removing the invalid binding. Notice how, when the connection is broken, we have no block settings — and, consequently, no attributes panel — in the sidebar:
Let's say you install a plugin with a custom binding source, make a few connections, but later decide you no longer want to use the plugin. How are admins supposed to not just be alerted to the broken bindings, but given a way to resolve them?
If we do remove the button for the non-admin users, we could give them a slightly different warning. Maybe something like, "Attribute "content" is connected to undefined "core/undefined" block bindings source. You have insufficient permissions to modify it. Please contact your site administrator."
SantosGuillamot
force-pushed
the
add/warning-in-bindings-with-undefined-source
branch
from
8ef9f15
to
c554b00
Compare
|
As discussed here, I believe there is one remaining decision before merging this pull request: Should we include a button to remove the invalid connection in this initial version, or should we wait until we receive more feedback on how it should work? If we decide to keep the button: Is it okay to show the button to any user with edit access or only admins? Additionally: Is the warning message copy clear enough? Personally, I lean towards keeping the button as it is, available to any user with edit access. But I'd love to know more opinions. For reference, this is what the warning looks like:
cc: @WordPress/gutenberg-design |
|
This is what I see when testing:
In general, showing error messages in the content is not the best idea. It can work when it's highly contextual. A few things, though. The text here is not the best:
What does "leave this block intact" actually mean? And how would I modify the connection? If modifying the connection happens in the inspector, probably this isn't the best place to actually show this error message, after all. The other aspect is, I can't select this as I can other blocks. It's almost as if it stops being a block. This is different from parser error, for example, where I can select this as a block:
You can test that state easily by pasting this into the code editor: The main reason that's important is that I can select the block, delete it, or press Enter to insert something after. With the text case outlined here, I have to go into the list view in order to insert blocks before or after, because I can't select the error and press "Enter" to add a paragraph below. If the error is best solved in the inspector, I'd approach this differently, and show the paragraph in this state:
That is: no special treatment, that's just a default paragraph. No action items in the canvas. Just the error message. And then you could show all sorts of error messages here:
Especially in the flyout, you could show ways to remove the connection, delete it, fix it, or otherwise. What do you think? |
|
Thanks a lot for the testing and feedback 🙂 You are right, and the place to "solve" this would be going to the code editor and changing the binding or removing the connection through the call to action we define. And it's true that it might not be accessible to everyone.
What happens when it is not the paragraph content the one connected to an invalid source? For example, an image title or an image URL. Do you think fixing the selection issue, changing the text to something like what you shared, and potentially keep the panel to create/modify connections would be better? |
Right, good point. Would it be possibl for you to use the existing |
Do you mean this one?
I am not sure if we should directly use it. These are my reasons:
For those reasons I would say that, if we want to through a warning, I would personally use the
Bear in mind that after this other pull request, the panel won't live in the paragraph settings tab, it will live in the unique tabs close to the "Advanced" section. The format is the same as Additionally, users could still see the bindings panel to connect to an existing source. Let me know what you think 🙂 |
|
Another possibility we could explore would be adding the recovery options as secondary actions:
|
|
I made a couple of commits: one to fix the selection issue and another to see how it would look without the button. With the current code:
Personally, I believe this is enough and we can always add a button later after receiving feedback. But I'd love to know more opinions. |
|
I guess you can't customize the text or buttons of those warnings, just found that out as part of working on #64997. But the point is, design-wise it should not feel different, and ideally use the same CSS. Can the componentry be updated, refined to address the use case in point here? |
|
I'm not sure if I follow. Design-wise they should feel the same and they use the same CSS. Both of the use the Warning component, which is the one defining the CSS: link. It seems So my question is: are we supposed to use |
|
Ah, either, I'll definitely refer to you on best practices. I'm mainly noting that, design glitches aside, there's an existing flow for "broken blocks" that should be replicated here, if we go with the in-canvas version. |
Could you describe the "broken blocks" flow to ensure it is replicated? I thought this was the expected workflow because it seems to be used in other places. Other examples could be: Template part doesn't exist Type not supported in query title Post content or post excerpt not available Comments not enabled in comments form
I'm happy to explore other options |
|
A few notes, when having: <!-- wp:paragraph {"metadata":{"bindings":{"content":{"source":"core/undefined"}}}} -->
<p>Default content</p>
<!-- /wp:paragraph -->This will still render In some cases, there are multiple attributes that can be connected to the source. In the case of Paragraph, it's the |
Yep, try this code: |
I've created this alternative pull request that instead of breaking the block it:
That feels less invasive, simpler and safer than this approach. I'm happy to pursue that path and figure out later based on feedback if a more prominent warning is needed. Let me know what you think. |
|
I'm closing this in favor of #65002. It seems to be enough for the first iteration. We can always reopen this PR or reuse the code if we decide to follow this approach based on feedback. |
What?
Now that the sources registered in the server are bootstrapped in the client after this pull request, I believe it is safe to assume that if the source doesn't exist in the editor, it is an error.
In this pull request, I'm adding a warning while checking the bindings.
Why?
It should help notify users that the block won't possibly work as expected.
How?
While going through the bindings, checks if the source is registered and if not, it throws a warning and provide a button to remove the connection.
Testing Instructions
Check that the warning is shown.
Click on "Remove connection" and check it shows the default content.
You can repeat the process with an image with multiple attributes: