-
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
Add blockEditor settings on the widget screen #15788
Add blockEditor settings on the widget screen #15788
Conversation
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.
lib/widgets-page.php
Outdated
@@ -29,9 +29,14 @@ function gutenberg_widgets_init( $hook ) { | |||
return; | |||
} | |||
|
|||
$block_editor_settings = apply_filters( 'block_editor_settings', $editor_settings, $post ); |
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.
Usually apply_filters
needs a doc block which documents the filter. Not sure why the linter hasn't complained here.
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.
Do we really need the settings and the filter to be the same between the two pages? I think adding the settings raises a lot of questions (what settings do we want to share and what settings are specific to the post editor). I think this answer should be addressed iteratively and settings added step by step as needed.
useEffect( () => { | ||
setupWidgetAreas(); | ||
}, [] ); | ||
return <Layout />; | ||
const blockEditorSettings = useMemo( |
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.
😍
*/ | ||
const EMPTY_OBJECT = {}; | ||
const WidgetBlockEditorSettings = createContext( EMPTY_OBJECT ); | ||
export default WidgetBlockEditorSettings; |
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.
In other parts of Gutenberg (e.g. DropZoneProvider
) we export default
the Provider
and expose Consumer
as a named export. I think it makes sense to be consistent.
So, that is, I suggest we rename this file to widget-block-editor-settings-provider/index.js
and go:
const { Provider, Consumer } = createContext( EMPTY_OBJECT );
export default Provider;
export { Consumer };
/** | ||
* Reference to an empty object | ||
*/ | ||
const EMPTY_OBJECT = {}; |
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.
Why use a constant?
lib/widgets-page.php
Outdated
@@ -29,9 +29,14 @@ function gutenberg_widgets_init( $hook ) { | |||
return; | |||
} | |||
|
|||
$block_editor_settings = apply_filters( 'block_editor_settings', $editor_settings, $post ); |
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.
$editor_settings
and $post
are undefined.
Notice: Undefined variable: editor_settings in /var/www/html/wp-content/plugins/gutenberg/lib/widgets-page.php on line 32
Notice: Undefined variable: post in /var/www/html/wp-content/plugins/gutenberg/lib/widgets-page.php on line 32
> | ||
<BlockList /> | ||
</BlockEditorProvider> | ||
<WidgetBlockEditorSettings.Consumer> |
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 could be useContext
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createContext } from '@wordpress/element'; |
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.
Using a context for the widgets settings does make sense if we forsee us having a complex nested component tree for the widgets page. Is this the case or will be just passing the settings down to the block editors?
Also, I want to remind that we did start with "Context" in the intial versions of the editor settings as well and we ultimately changed to use the data module because we needed to make updates to the settings from the component and we didn't want to reinvent the wheel as this just meant we need global state that can be updated (data store).
As always, I like to start with the simplest possible solution. Isn't a "settings" prop a solution?
This PR was updated we now pass the settings down in the components instead of using components, and we only pass a small subset of settings (that we will need for sure) from the server until we decide what should be passed or not. |
5a863f7
to
4ae9c2a
Compare
…ver. (+1 squashed commit) Squashed commits: [46e57de] Add blockEditor settings on the widget screen; Required to make legacy widgets work.
4ae9c2a
to
bb20459
Compare
Hi @noisysocks, I'm not being able to reproduce the problem. When I add the legacy widgets block I see some widgets available, (they can still not be used because of other bugs). Do you have any widget installed? (core widgets are hidden because we have equivalent blocks) I'm doing some tests with https://wordpress.org/plugins/meks-smart-author-widget/. |
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 not seeing the warning any more when I test. Maybe I didn't have the right branch checked out...
Anyway, this looks good!
lib/widgets-page.php
Outdated
} | ||
|
||
$color_palette = current( (array) get_theme_support( 'editor-color-palette' ) ); | ||
$font_sizes = current( (array) get_theme_support( 'editor-font-sizes' ) ); |
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.
What is the current()
here for? It's a fairly low-level array function so I'm surprised to see it. We probably just want to get the first item in the array.
list( $color_palette, ) = (array) get_theme_support( 'editor-color-palette' )
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.
Fixed in 0e854bb.
The block editor settings are required to ensure legacy widgets work as expected on the widgets block editor screen.
This PR is related to #15521 given that in both PR's we add block editor settings support to the widget screen.
@youknowriad referred in #15521 that saving the block editor settings in the widgets store too. This PR uses context as an alternative approach.
Description
I added a new legacy widget block to the widget block editor page.
I verified that the legacy widget loads the placeholder normally, instead of a message saying the user has no permissions to manage widgets.