Add 'Widget Group' block to widgets screens #34484
Merged
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
noisysocks
added
[Type] Enhancement
noisysocks
requested review from
adamziel,
draganescu,
kevin940726,
spacedmonkey,
talldan,
tellthemachines and
TimothyBJacobs
as code owners
11 tasks
|
Size Change: -722 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
kevin940726
reviewed
Sep 2, 2021
|
Nice! This seems to work well. A couple of notes:
Side note: I wonder if we should update the Legacy Widget editing UI to incorporate the heading/title format used here? In that case some inputs would be blank (e.g. Image) and some would have the placeholder hint (e.g. Archives), I believe.. |
noisysocks
requested review from
ajitbohra and
chrisvanpatten
as code owners
Good idea. Done in 1594a24e44f41483a30dfffeb28847ce1bfb16c9.
Done in e92cdb2310736b4a4f23aff840a7623fb48cb688.
Yep, makes sense. Done in 12475d4c2a6071f90b7ab3492a7610576bdd4400.
This would have to be changed in Core since that's where the forms for editing these widgets comes from. |
Co-authored-by: Kai Hao <[email protected]>
noisysocks
force-pushed
the
add/widget-group
branch
from
6bf3b0e to
4abd4e8
Compare
noisysocks
added
Backport to WP Minor Release
|
OK this should be ready for re-review now! 🌻 It's too late for this to be included in 5.8.1, I think, as ideally it needs time in the plugin to receive testing. I've put the |
|
Awesome, looking good.
This probably needs more thought anyway — maybe something to revisit later 😅 Side note that it's difficult to test this with a keyboard in the Customizer — it seems like #32175 may have regressed at some point since it was merged? |
Hm, I'm not seeing this. Could you tell me more? I'm able to insert and edit a Widget Group using the keyboard and when I press Esc it puts me in block selection mode as expected. Kapture.2021-09-06.at.15.36.46.mp4 |
Ok looks like an issue on my end, thanks for double checking! |
|
I tested this and it works great. I have two issues with how it "feels":
|
There was a problem hiding this comment.
This is working well, aside from a couple of odds and ends I commented on below.
There's also a slight issue in the Customizer: adding any block inside a Widget Group causes Uncaught TypeError: Cannot read properties of null (reading 'data') to show in the console. Seems to be coming from here.
It feels a bit weird to have an un-editable title above the blocks; I keep clicking it and being surprised when all the blocks disappear. But I don't have any better suggestions either 😅
Hm I'm not sure. "Block with title" reads to me like a pattern name. Also, Widget Group supports multiple blocks so it isn't really accurate to say "Block". What do you think, @critterverse?
I'll defer to @critterverse here too 🙂 Here's what it looks like if we show the inner blocks while editing the title. Kapture.2021-09-07.at.11.03.14.mp4Alternatively we could make the title directly editable. This might cause confusion as the title is not truly WYSIWYG, but perhaps the placeholder properly indicates that this block is "different". Kapture.2021-09-07.at.11.12.16.mp4 |
@tellthemachines: Hm I'm not seeing this. What WP version are you running? Does it happen with Gutenberg trunk or only this PR? |
Oh, it appears I'm on 5.8-beta2-51213 😕 though I ran |
|
Ok I retested with 5.7, 5.8 and 5.9-alpha-51732 and can't reproduce the issue so it should be fine 😄 |
tellthemachines
approved these changes
Sep 7, 2021
Sounds like a plan! @critterverse: Let me know what you think about #34484 (comment) and I can make make any changes we want in a follow-up PR. |
|
Thanks for the feedback, all. The Legacy Widget-like toggling between editing and preview mode seems to be what people are finding jarring in this context, so I'm thinking we should go with this approach after all:
^See the second video in #34484 (comment) There are still some awkward things with this approach as discussed here and in #33881 but as @noisysocks mentioned, at least the placeholder helps. |
7 tasks
Mamaduka
removed
the
Backport to WP Minor Release
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.
Description
Closes #32723.
Adds a new block called Widget Group to the widget block editors. This lets users add a title to a group of blocks in a way that's compatible with older themes. You can read through the above linked issue for more context on the problem that this attempts to address.
In this PR I've taken #33881 and implemented the flow described by @critterverse in #33881 (comment).
Why patterns don't work here
A new block was created instead of utilising patterns. This was explored in #33881 but ultimately it was decided it didn't solve the core user problems.
The key problems were:
core/headingfor the title.The decision about this can be found here.
How has this been tested?
Screenshots
Kapture.2021-09-02.at.14.53.22.mp4
Checklist:
*.native.jsfiles for terms that need renaming or removal).