-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Block bindings: Lock editing in fields in editor if meta fields panel is opened. #64738
Block bindings: Lock editing in fields in editor if meta fields panel is opened. #64738
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. |
Size Change: +622 B (+0.04%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
I like where it is heading. Thank you for opening this alternative PR. We will have to cross-check with #64570 that adds a new preference for enabling/disabling Block Bindings UI. |
Flaky tests detected in cc4009d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10698331292
|
I see that preference slightly different because they could be compatible. If we follow this approach, what would we want?
So I guess it is a matter of deciding if we want only the first option or both of them. First option seems enough for me, but I don't have a strong opinion. |
Apart from that, I like the approach. My main concern is that it solves the issue faced in block bindings but other plugins like this report will still face the same problem when they modify post meta through the store. Additionally, other external sources like ACF, Pods, or WooCommerce will probably need to replicate this logic. |
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.
<CustomFieldsConfirmation willEnable={ isChecked } /> | ||
) } | ||
</PreferenceBaseOption> | ||
<> |
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 believe this extra wrapper is not needed, right?
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 tried different approaches and forgot to remove it. 😓
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.
Done in 754a174
{ isChecked && ( | ||
<p className="edit-post-preferences-modal__custom-fields-confirmation-message"> | ||
{ __( | ||
'Connected attributes cannot be edited in the block editor while this interface is active.' |
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.
Should this be something like 'Custom fields connected to block attributes cannot be edited in the block editor while this interface is active.' or something similar?
Technically speaking, users aren't editing the attributes but the custom fields.
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.
Done in 754a174
8a02ae7
to
754a174
Compare
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 works as expected for me 🙌
I do have a comment on the wording of the message.
<p className="edit-post-preferences-modal__custom-fields-confirmation-message"> | ||
{ __( | ||
'Custom fields connected to block attributes cannot be edited in the block editor while this interface is active.' | ||
) } |
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.
For me, this wording is still hard to follow. How about a phrasing like this: "Enabling this interface will disable inline editing of custom fields."
The reason I say this: The meta fields panel is still part of the block editor too — at least, It appears that way to an end user. With that in mind, I think specifying 'inline editing' as becoming disabled is clearer.
Additionally, we can likely further simplify the message by removing the 'connected to block attributes' detail. Without the meta fields panel, it's only possible to edit custom fields in the editor when they're connected, right? So to me, specifying extra details around connections just makes the message verbose and difficult to understand.
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.
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 share similar concerns regarding the UX here. I'm in favor of simplifying the message or even looking for an alternative approach (but not sure what).
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.
No change to the message works for me, too. The UI will lock everything as in WordPress 6.5 which will be the clear indicator that it can't be edited inline. One potential enhancement to consider would be showing a prompt to disable Meta Boxes / Custom Fields interface if folks want to use the Attributes in the sidebar. This way, they could reload the page switch between modes. Not the best user experience, but it explains the story well.
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'm fine removing the message as well. And we can figure out later if it is needed, or if the Attributes panel is a better place, based on feedback.
What?
Fixes #64217
Following @gziolo proposal, let's try an experiment that locks editing if the meta fields panel is opened.
An alternative to #64505
Why?
Experimenting.
How?
Testing Instructions
Screenshots or screencast
Screen.Recording.2024-08-23.at.12.35.54.mov