Fix showing double icons for connected blocks in pattern editor #62317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevin940726
added
[Type] Bug
|
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: +122 B (+0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
talldan
reviewed
Jun 5, 2024
Comment on lines
178
to
183
| { isUsingBindings && | ||
| hasPatternOverrides && | ||
| canBindBlock( blockName ) && ( | ||
| <BlockBindingsIndicator clientIds={ blockClientIds } /> | ||
| ) } |
There was a problem hiding this comment.
I think the BlockBindingsIndicator should be visible when using other types of bindings (e.g. meta) outside of the pattern block, so by adding hasPatternOverrides to the condition does it prevent that?
There was a problem hiding this comment.
Good point. I just pushed 5ebaab8. It'll show double icons for post meta but I guess we need that for now?
There was a problem hiding this comment.
Hmm, now I'm not sure what's expected for bindings. It should be one block icon, and I guess a purple one?
I'm not completely up to date on the block bindings UI changes, so would be good to get some confirmation.
There was a problem hiding this comment.
Until now, the purple icon was just a connection icon.
I can see a duplicated icon in trunk now, I guess since the linked pull request. I would personally keep the current UX showing the connection icon rather than the duplicated one, and figure out later how to keep evolving the UX based on feedback. But it'd be great to hear from @richtabor and @jasmussen .
There was a problem hiding this comment.
I prefer the after here, honestly. We've tried out the connection icon in this iteration in trunk for a bit, but it hasn't felt like it added clarity. I think there are other things we can do, but for now, I'd omit it.
There was a problem hiding this comment.
That looks right to me, yes. The "This block is connected" message can be integrated into the transforms dropdown if need be.
There was a problem hiding this comment.
Perfect 🙂 @kevin940726 Let us know if it makes sense, if you plan to work on that, and if you need any help!
There was a problem hiding this comment.
Thanks! I'm working on it now. Will push a commit soon!
There was a problem hiding this comment.
I just pushed c862489. It now has the following UI:
Post meta connected block:
Pattern overrides connected block in focus mode:
Pattern overrides connected block in post:
LMK if I miss something!
There was a problem hiding this comment.
Regarding the UX, I believe it is much better now! Thanks for working on it. I'll take a look at the code asap.
|
Flaky tests detected in c862489. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9415176442
|
kevin940726
removed
the
Backport to WP 6.7 Beta/RC
|
I removed the backport label until we reach a design decision, and I'll add it back when it's merged. |
|
Should this be backported for WP 6.6? If so, please add the |
kevin940726
force-pushed
the
fix/binding-double-icons
branch
from
1b51b08
to
c862489
Compare
kevin940726
added
the
Backport to WP 6.7 Beta/RC
| @@ -22,13 +22,10 @@ import useBlockDisplayTitle from '../block-title/use-block-display-title'; | |||
|
|
|||
| export default function BlockBindingsToolbarIndicator( { clientIds } ) { | |||
There was a problem hiding this comment.
If I am not mistaken, this component becomes specific to pattern overrides because it is only used when adding a pattern in a post, right? Maybe we can rename it and slightly change the implementation to reflect that?
There was a problem hiding this comment.
On second thought, I don't think this even belongs in the block-editor package 😅. I'd imagine it'll be a bigger refactor if we figure out a way to move it to the patterns package though. I'd prefer to do it in a follow-up PR if necessary. WDYT?
There was a problem hiding this comment.
Moving it to the patterns package seems logical to me 🙂 And I guess it is okay to do it in a follow-up PR. Should we create an issue or something to ensure it is not lost?
Maybe it is still worth changing the component name to clarify it is related to patterns and not block bindings until that PR is done?
There was a problem hiding this comment.
Done in 72d0a8e! I renamed some stuff, so could appreciate a final review to ensure I didn't break anything 😆.
| @@ -118,6 +121,17 @@ function BlockSwitcherDropdownMenuContents( { | |||
| </p> | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
I'm wondering if we could get isUsingBindings from this useSelect. Something similar to what is being done here.
If we do this, I believe we could avoid passing isUsingBindings to BlockSwitcher and BlockSwitcherDropdownMenuContents components.
Additionally, I think we wouldn't need isUsingBindings in the block toolbar anymore (here) and we could just have something like hasPatternOverrides, as it is now specific to pattern overrides, right?
I am not 100% sure if it makes sense, but maybe it is worth taking a look.
There was a problem hiding this comment.
I believe so, yes! However I don't think it makes much difference. Currently we still need isUsingBindings in block toolbar to apply the is-connected class name. We can somehow move the class name to block switcher, but in terms of performance I don't think it matters that much. Do you think it's worth exploring?
There was a problem hiding this comment.
I wasn't thinking about performance but about readability, but I don't have a strong opinion, so whatever you decide it's okay for me 🙂
There was a problem hiding this comment.
I think it reads okay for me, but I'm obviously biased 😅. Happy to do some further improvements if we find something confusing later! 🙇
kevin940726
force-pushed
the
fix/binding-double-icons
branch
from
c702ddb
to
72d0a8e
Compare
SantosGuillamot
approved these changes
Jun 11, 2024
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…Press#62317) Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> * Fix showing double icons for connected blocks in pattern editor * Fix post-meta * Unify the experience * Rename the component
29 tasks
ellatrix
pushed a commit
that referenced
this pull request
Jun 18, 2024
Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> * Fix showing double icons for connected blocks in pattern editor * Fix post-meta * Unify the experience * Rename the component
ellatrix
pushed a commit
that referenced
this pull request
Jun 18, 2024
Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> * Fix showing double icons for connected blocks in pattern editor * Fix post-meta * Unify the experience * Rename the component
|
I just cherry-picked this PR to the wp/6.6-beta-3 branch to get it included in the next release: a052b4c |
ellatrix
added
Backported to WP Core
ellatrix
pushed a commit
that referenced
this pull request
Jun 18, 2024
Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> * Fix showing double icons for connected blocks in pattern editor * Fix post-meta * Unify the experience * Rename the component
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Continued from #61560. Fix #61560 (comment).
Prevent double icons from showing when editing the synced pattern itself.
Why?
Showing double icons is confusing to the users.
How?
Only show the binding indicator when editing an overridable pattern as suggested in the issue.
Testing Instructions
Screenshots or screencast
Before:
After:
