Display wide widgets as popovers in Customizer #31736
Conversation
tellthemachines
requested review from
talldan,
noisysocks,
kevin940726 and
critterverse
|
Size Change: -822 kB (-44%) 🎉 Total Size: 1.03 MB
ℹ️ View Unchanged
|
skorasaurus
added
the
[Feature] Widgets Customizer
|
Woo, this is a great start. Here's what I'm seeing:
A couple of initial notes:
|
| </div> | ||
| </Popover> | ||
| ); | ||
| } |
There was a problem hiding this comment.
We can remove the bit of code duplication here by doing something like:
let form = (
<div
ref={ ref }
className="wp-block-legacy-widget__edit-form"
hidden={ ! isVisible }
>
<h3 className="wp-block-legacy-widget__edit-form-title">
{ title }
</h3>
</div>
);
if ( isWide && isMediumLargeViewport ) {
form = <Popover focusOnMount={ false } position="bottom left">{ form }</Popover>;
}
return form;
There was a problem hiding this comment.
We can do early return in the if condition instead of reassigning variable too.
There was a problem hiding this comment.
This will no longer work because I changed the markup when isWide to render the title outside of the Popover.
| @@ -194,6 +194,11 @@ export default class SidebarAdapter { | |||
| const settingId = widgetIdToSettingId( widgetId ); | |||
| const setting = this.api( settingId ); | |||
|
|
|||
| const isWide = wp.customize.Widgets.data.availableWidgets.filter( | |||
| // eslint-disable-next-line camelcase | |||
There was a problem hiding this comment.
Another (perhaps cleaner) way of working around this lint rule is to not destructure.
const isWide = availableWidgets.filter( ( widget ) => widget.id_base === idBase );| @@ -17,6 +17,10 @@ | |||
| "instance": { | |||
| "type": "object", | |||
| "default": null | |||
| }, | |||
| "isWide": { | |||
There was a problem hiding this comment.
I'm not 100% sure this should be an attribute because it's not "content" per se. For example, if a Legacy Widget block is in a post, and isWide is toggled somehow, should that create an undo/redo level in the block editor?
Legacy Widget has idBase available to it. Could we have the Legacy Widget block itself decide whether or not it should be wide or not by accessing wp.customize.Widgets.data.availableWidgets from packages/block-library/src/legacy-widget/edit/index.js? The block already heavily relies on older WordPress APIs so I don't think it's a problem to do this. It will just have to be careful to account for the case that wp.customize is undefined.
There was a problem hiding this comment.
Another option would be to add is_wide to the /widget-types REST API endpoint. Probably not necessary though.
There was a problem hiding this comment.
Not sure I follow, if we added is_wide to the endpoint wouldn't we end up having to add it to the block attributes too?
There was a problem hiding this comment.
So trying to get isWide directly inside the Legacy Widget block through wp.customize... works well, and fixes an issue I was having with widgets not having the value set when they're first added to the screen. But now I have to somehow figure out if the block is rendering in the Customizer or not, because we only want to show the popover in the Customizer, not in the Widgets screen. Aside from the general yuckiness of having a block be aware of where it's being rendered, is there any way of doing this that doesn't involve checking the current URL?
Or... I dunno. isWide is as likely to change as idBase. I don't think it's too awful to have it in attributes. Or is there another way?
There was a problem hiding this comment.
There's also the option of using a BlockEdit filter to help implement this. That filter will have access to all the attributes for the block and would be implemented only in the customizer.
An extreme option—not sure if doing something like putting the whole BlockEdit in a popover using the filter would work:
{ isWide && (
<Popover>
<BlockEdit { ...props } />
</Popover>
) }Seems a little crazy, but would keep the legacy widget block implementation editor agnostic. The lighter alternative might be passing an isWide prop down to BlockEdit in the filter, which the block can use.
The other option I could think of is to implement the feature conditionally based on the availability of the wp.customize api, which isn't defined in the standalone editor. That would break if it ever were available.
There was a problem hiding this comment.
Ha, turns out I was doing something wrong and isWide was always true, so that's why popovers were rendering in the Widgets screen 😂
Ooooh BlockEdit, good tip! I'll look into it.
There was a problem hiding this comment.
Ok so putting BlockEdit inside the Popover won't work because we only want to render a part of the form in the popover, not the whole edit. But I added a filter to pass in isWide which seems much cleaner.
There was a problem hiding this comment.
Sorry, I went MIA 😅
Not sure I follow, if we added is_wide to the endpoint wouldn't we end up having to add it to the block attributes too?
No, because we access the widget-types endpoint from within Edit.
But now I have to somehow figure out if the block is rendering in the Customizer or not, because we only want to show the popover in the Customizer, not in the Widgets screen. Aside from the general yuckiness of having a block be aware of where it's being rendered, is there any way of doing this that doesn't involve checking the current URL?
wp.customize will be undefined if the block is not in the Customizer.
There was a problem hiding this comment.
I looked into how is_wide is implemented in Core and I don't think it makes sense to add is_wide to the REST API because the logic is so specific to the Customizer.
So I think your current approach in this PR is the right one.
|
hey @critterverse , thanks for the feedback! I've updated the markup to display the title outside of the Popover. In regards to container height, I made it a minimum 30px so it occupies a little space, but can't make it any bigger because the popover sometimes displays under it, so making it higher would just add a bunch of empty space in the editor. The buggy popover positioning is a known issue (#21275, #29868) and will have to be addressed separately. The blue line you are seeing is the focus indicator, but because the container had no height it was looking a bit odd. Hopefully now it's better. Keyboard focus stays on the container when the popover is opened, and tabbing once moves into the popover. That part works like in the old customizer widgets. The main difference in keyboard behaviour between old and new is that tabbing out of the popover in the new version doesn't bring us to the next widget/block, but follows block editor keyboard behaviour in moving focus to the bottom of the editor (in the customizer that translates to the "hide contols" button). Any thoughts on this? |
There was a problem hiding this comment.
Code looks good! I think the approach you took is the right one.
Here's how it looked when I tested. The popover positioning is definitely not great. I wonder if it would be better to just use overflow: scroll until we implement #21275. I'm not sure 😕
| ? wp.customize.Widgets.data.availableWidgets.filter( | ||
| ( widget ) => widget.id_base === idBase | ||
| )[ 0 ]?.is_wide | ||
| : false; |
There was a problem hiding this comment.
We can make this easier to read by swapping filter for find and using ?. and ??.
const isWide =
wp?.customize?.Widgets.data.availableWidgets.find(
( widget ) => widget.id_base === idBase
)?.is_wide ?? false;And, because this is @wordpress/customize-widgets, we can safely assume that wp.customize is defined.
const isWide =
wp.customize.Widgets.data.availableWidgets.find(
( widget ) => widget.id_base === idBase
)?.is_wide ?? false;| { ! id && ! idBase ? ( | ||
| <Empty { ...props } /> | ||
| ) : ( | ||
| <NotEmpty { ...props } /> | ||
| <NotEmpty { ...props } isWide={ isWide } /> |
There was a problem hiding this comment.
The isWide={ isWide } here is unnecessary as isWide is already going to be passed down via ...props.
| @@ -77,6 +90,7 @@ function NotEmpty( { | |||
| setAttributes, | |||
| clientId, | |||
| isSelected, | |||
| isWide, | |||
There was a problem hiding this comment.
Will need to default this to false if you remove the isWide={ isWide } per my other comment.
|
This is a tricky one, I’m not sure this design is going to work using popovers if we're unable to position them consistently. I'm seeing this: oversized-legacy-widgets.mov
^ The design also kind of breaks down and becomes visually confusing without being able to implement this aspect that would help anchor the popover container.
|
tellthemachines
force-pushed
the
add/popover-wide-widgets
branch
from
beea27a to
9b57798
Compare
|
@tellthemachines I shared an alternate design direction in #32210 that might help us get around some of the popover issues we're running into here. Let me know what you think! |
|
Thanks @critterverse , I left a comment on the issue. I think for now our best bet is still the popover, as it won't require any significant code changes. I made some adjustments to its positioning so that it now renders always on the right hand side, over the preview, and let the popover component determine whether it should go above or below the block, depending on its position in the viewport. This is pretty close to current behaviour, so shouldn't be too unexpected:
|
noisysocks
added
Backport to WP 6.7 Beta/RC
|
In Firefox there's a bit of unnecessary padding above the "No preview available." placeholder when you hover over the block.
Other than that, let's get this merged as it's better than what we have in |
tellthemachines
force-pushed
the
add/popover-wide-widgets
branch
from
1927312 to
c5f0f38
Compare
Fixed! |
Agree! 👍 Can we also unify the height on the "editing mode" block container so that it has the same height/padding across preview and editing mode?
|
|
I think we need some responsive mobile viewport logic to move the form back inline when the browser is resized. At the moment the form seems to disappear when making the browser narrow and then wide again: If a wide widget block is added when the viewport is already at a mobile size, the form does appear inline correctly, so hopefully it's just a little bit of logic to adjust. |
I've made the container height equal to the minimum height of the "no preview available" placeholder. This won't always match the height of the block in preview mode, as a widget with a really long title could stretch the "no preview" placeholder, and the previews, when they exist, have varying heights. But it'll make the empty default a bit more consistent.
Fixed! It's probably worth mentioning that the media queries we're using won't always match whatever the third-party widgets have defined, so there might be a little weirdness in the in-between sizes. E.g. for SiteOrigin widgets, between 600 and 680px, this happens:
|
tellthemachines
force-pushed
the
add/popover-wide-widgets
branch
from
eb81f02 to
b04cae5
Compare
|
Last couple of comments from me! Huge thanks @tellthemachines for all the hard work and iterating on this. Seeing some extra space above the placeholder preview when the block is focused in Navigation/Select mode:
Also I was able to open overlapping popovers by clicking the sibling inserter while the oversized widget popover is open. Not sure what kind of control we have here but if possible it would be great to force the widget popover closed if that is clicked:
|
Thanks, this is now fixed!
I initially thought this might be due to how we're handing focus between the popover and the rest of the page, but changing the settings on the popover doesn't fix this, so it must be something else. I haven't been able to work out where the problem is yet, so we might want to fix it separately so as not to delay this PR further. |
There was a problem hiding this comment.
Much better than what's in trunk! Let's get this in and work on #32210 after 5.8.
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
This includes the following fixes: Widgets Editor: - Load widgets.php WordPress/gutenberg#32299 - Fix Legacy Widget Preview WordPress/gutenberg#32300 - Fix error when saving empty Legacy Widget block WordPress/gutenberg#32359 Widget blocks in the customizer: - Fix deselection blocks when the inspector is open WordPress/gutenberg#32361 - Display wide widgets as popovers WordPress/gutenberg#31736 Global Styles: - Align classNames generation between client and server WordPress/gutenberg#32352 - Group typography block supports WordPress/gutenberg#32252 WordPress/gutenberg#32444 WordPress/gutenberg#32459 - Make theme.json syntax errors more visible to the users WordPress/gutenberg#32404 Template Editor: - Update the appearance of the template details WordPress/gutenberg#32042 - Fix layout definition WordPress/gutenberg#32425 - Fix grouping post content block WordPress/gutenberg#32453 Miscellaneous: - Prevent saving when the post is locked WordPress/gutenberg#32341 - Fix allowed block patterns selector WordPress/gutenberg#32376 - Fix wrong results in the Post Author picker WordPress/gutenberg#32344 - Fix notices position in top toolbar mode WordPress/gutenberg#32238 - Allow non-latin characters in post slugs WordPress/gutenberg#32232 - Fix Random collapse of the color settings panel WordPress/gutenberg#32388 - Fix theme logo theme mode not being removed on theme removal WordPress/gutenberg#32370 - Fix block alignment styles in the editor WordPress/gutenberg#32454 - Fix some block toolbar overlaps WordPress/gutenberg#32424 - Fix content loss when switching list types WordPress/gutenberg#32432 Performance: - Improve the performance of buttons block WordPress/gutenberg#32356 - Improve the performance of the container blocks WordPress/gutenberg#32380 Props noisysocks, nosolosw, jorgefilipecosta. See #52991. git-svn-id: https://develop.svn.wordpress.org/trunk@51089 602fd350-edb4-49c9-b593-d223f7449a82
This includes the following fixes: Widgets Editor: - Load widgets.php WordPress/gutenberg#32299 - Fix Legacy Widget Preview WordPress/gutenberg#32300 - Fix error when saving empty Legacy Widget block WordPress/gutenberg#32359 Widget blocks in the customizer: - Fix deselection blocks when the inspector is open WordPress/gutenberg#32361 - Display wide widgets as popovers WordPress/gutenberg#31736 Global Styles: - Align classNames generation between client and server WordPress/gutenberg#32352 - Group typography block supports WordPress/gutenberg#32252 WordPress/gutenberg#32444 WordPress/gutenberg#32459 - Make theme.json syntax errors more visible to the users WordPress/gutenberg#32404 Template Editor: - Update the appearance of the template details WordPress/gutenberg#32042 - Fix layout definition WordPress/gutenberg#32425 - Fix grouping post content block WordPress/gutenberg#32453 Miscellaneous: - Prevent saving when the post is locked WordPress/gutenberg#32341 - Fix allowed block patterns selector WordPress/gutenberg#32376 - Fix wrong results in the Post Author picker WordPress/gutenberg#32344 - Fix notices position in top toolbar mode WordPress/gutenberg#32238 - Allow non-latin characters in post slugs WordPress/gutenberg#32232 - Fix Random collapse of the color settings panel WordPress/gutenberg#32388 - Fix theme logo theme mode not being removed on theme removal WordPress/gutenberg#32370 - Fix block alignment styles in the editor WordPress/gutenberg#32454 - Fix some block toolbar overlaps WordPress/gutenberg#32424 - Fix content loss when switching list types WordPress/gutenberg#32432 Performance: - Improve the performance of buttons block WordPress/gutenberg#32356 - Improve the performance of the container blocks WordPress/gutenberg#32380 Props noisysocks, nosolosw, jorgefilipecosta. See #52991. git-svn-id: https://develop.svn.wordpress.org/trunk@51089 602fd350-edb4-49c9-b593-d223f7449a82
This includes the following fixes: Widgets Editor: - Load widgets.php WordPress/gutenberg#32299 - Fix Legacy Widget Preview WordPress/gutenberg#32300 - Fix error when saving empty Legacy Widget block WordPress/gutenberg#32359 Widget blocks in the customizer: - Fix deselection blocks when the inspector is open WordPress/gutenberg#32361 - Display wide widgets as popovers WordPress/gutenberg#31736 Global Styles: - Align classNames generation between client and server WordPress/gutenberg#32352 - Group typography block supports WordPress/gutenberg#32252 WordPress/gutenberg#32444 WordPress/gutenberg#32459 - Make theme.json syntax errors more visible to the users WordPress/gutenberg#32404 Template Editor: - Update the appearance of the template details WordPress/gutenberg#32042 - Fix layout definition WordPress/gutenberg#32425 - Fix grouping post content block WordPress/gutenberg#32453 Miscellaneous: - Prevent saving when the post is locked WordPress/gutenberg#32341 - Fix allowed block patterns selector WordPress/gutenberg#32376 - Fix wrong results in the Post Author picker WordPress/gutenberg#32344 - Fix notices position in top toolbar mode WordPress/gutenberg#32238 - Allow non-latin characters in post slugs WordPress/gutenberg#32232 - Fix Random collapse of the color settings panel WordPress/gutenberg#32388 - Fix theme logo theme mode not being removed on theme removal WordPress/gutenberg#32370 - Fix block alignment styles in the editor WordPress/gutenberg#32454 - Fix some block toolbar overlaps WordPress/gutenberg#32424 - Fix content loss when switching list types WordPress/gutenberg#32432 Performance: - Improve the performance of buttons block WordPress/gutenberg#32356 - Improve the performance of the container blocks WordPress/gutenberg#32380 Props noisysocks, nosolosw, jorgefilipecosta. See #52991. Built from https://develop.svn.wordpress.org/trunk@51089 git-svn-id: http://core.svn.wordpress.org/trunk@50698 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This includes the following fixes: Widgets Editor: - Load widgets.php WordPress/gutenberg#32299 - Fix Legacy Widget Preview WordPress/gutenberg#32300 - Fix error when saving empty Legacy Widget block WordPress/gutenberg#32359 Widget blocks in the customizer: - Fix deselection blocks when the inspector is open WordPress/gutenberg#32361 - Display wide widgets as popovers WordPress/gutenberg#31736 Global Styles: - Align classNames generation between client and server WordPress/gutenberg#32352 - Group typography block supports WordPress/gutenberg#32252 WordPress/gutenberg#32444 WordPress/gutenberg#32459 - Make theme.json syntax errors more visible to the users WordPress/gutenberg#32404 Template Editor: - Update the appearance of the template details WordPress/gutenberg#32042 - Fix layout definition WordPress/gutenberg#32425 - Fix grouping post content block WordPress/gutenberg#32453 Miscellaneous: - Prevent saving when the post is locked WordPress/gutenberg#32341 - Fix allowed block patterns selector WordPress/gutenberg#32376 - Fix wrong results in the Post Author picker WordPress/gutenberg#32344 - Fix notices position in top toolbar mode WordPress/gutenberg#32238 - Allow non-latin characters in post slugs WordPress/gutenberg#32232 - Fix Random collapse of the color settings panel WordPress/gutenberg#32388 - Fix theme logo theme mode not being removed on theme removal WordPress/gutenberg#32370 - Fix block alignment styles in the editor WordPress/gutenberg#32454 - Fix some block toolbar overlaps WordPress/gutenberg#32424 - Fix content loss when switching list types WordPress/gutenberg#32432 Performance: - Improve the performance of buttons block WordPress/gutenberg#32356 - Improve the performance of the container blocks WordPress/gutenberg#32380 Props noisysocks, nosolosw, jorgefilipecosta. See #52991. Built from https://develop.svn.wordpress.org/trunk@51089 git-svn-id: https://core.svn.wordpress.org/trunk@50698 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This includes the following fixes: Widgets Editor: - Load widgets.php WordPress/gutenberg#32299 - Fix Legacy Widget Preview WordPress/gutenberg#32300 - Fix error when saving empty Legacy Widget block WordPress/gutenberg#32359 Widget blocks in the customizer: - Fix deselection blocks when the inspector is open WordPress/gutenberg#32361 - Display wide widgets as popovers WordPress/gutenberg#31736 Global Styles: - Align classNames generation between client and server WordPress/gutenberg#32352 - Group typography block supports WordPress/gutenberg#32252 WordPress/gutenberg#32444 WordPress/gutenberg#32459 - Make theme.json syntax errors more visible to the users WordPress/gutenberg#32404 Template Editor: - Update the appearance of the template details WordPress/gutenberg#32042 - Fix layout definition WordPress/gutenberg#32425 - Fix grouping post content block WordPress/gutenberg#32453 Miscellaneous: - Prevent saving when the post is locked WordPress/gutenberg#32341 - Fix allowed block patterns selector WordPress/gutenberg#32376 - Fix wrong results in the Post Author picker WordPress/gutenberg#32344 - Fix notices position in top toolbar mode WordPress/gutenberg#32238 - Allow non-latin characters in post slugs WordPress/gutenberg#32232 - Fix Random collapse of the color settings panel WordPress/gutenberg#32388 - Fix theme logo theme mode not being removed on theme removal WordPress/gutenberg#32370 - Fix block alignment styles in the editor WordPress/gutenberg#32454 - Fix some block toolbar overlaps WordPress/gutenberg#32424 - Fix content loss when switching list types WordPress/gutenberg#32432 Performance: - Improve the performance of buttons block WordPress/gutenberg#32356 - Improve the performance of the container blocks WordPress/gutenberg#32380 Props noisysocks, nosolosw, jorgefilipecosta. See #52991. Built from https://develop.svn.wordpress.org/trunk@51089 git-svn-id: http://core.svn.wordpress.org/trunk@50698 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Description
Fixes #31194.
Using Popover component to display wide widgets on desktop screen in Customizer (for mobile the view is unchanged).
I disabled the managed focus for now, but keyboard behaviour isn't perfect. It's better than it is without the popover though, so we'll need to do some work on the mobile view too. Not sure if as part of this PR or not.
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).