-
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
Use interface on widgets screen sidebar #22304
Use interface on widgets screen sidebar #22304
Conversation
Size Change: +258 B (0%) Total Size: 835 kB
ℹ️ View Unchanged
|
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.
Nice to see how those component integrates so easily. Great job @jorgefilipecosta 😃
For the full picture, I haven’t tested this PR.
.blocks-widgets-container .editor-style-wrapper { | ||
max-width: $widget-area-width; | ||
margin: auto; | ||
} | ||
|
||
.edit-widgets-sidebar .components-button.interface-complementary-area__pin-unpin-item { |
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.
Was the prop to disable pinning feature ported over from edit-post
?
className="edit-widgets-sidebar" | ||
smallScreenTitle={ __( 'Widget Areas' ) } | ||
scope="core/edit-widgets" | ||
complementaryAreaIdentifier="edit-widgets/block-inspector" |
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 guess the renaming of this prop is in progress in other PR? The name is long and repeats mostly the name of the component.
@@ -4,6 +4,7 @@ export const DEFAULTS = { | |||
complementaryArea: { | |||
'core/edit-site': 'edit-site/block-inspector', | |||
'core/edit-post': 'edit-post/document', | |||
'core/edit-widgets': 'edit-widgets/block-inspector', |
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 do we need these here? A third-party plugin implementing his own area won't have access to this.
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 assumed you were fine with the structure of the reducer 😃
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 line just ensures a sidebar is open by default when we load a screen for the first time ever. On the follow loads we persist the sidebar state from the local storage.
But it is a good point if a plugin is implementing its own screen, it would not be able to set a default.
I will try to solve this issue and remove these defaults in a separate PR. I have two options on the slot I can add a prop that sets the name of the sidebar open for the first time ever, or on the complimentary area (fill) I set a boolean flag if the complementary should be the one open for the first time. Any preference on these two options?
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.
So basically a choice between a prop on the "slot" or on the "fill" 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.
Slots probably won't know what fills they have (if they come from plugins), so it seems like a prop on the fill might be better. The question becomes what happens if multiple fills do that.
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.
The advantage of the slot is that we would not have the problem of two fills specifying that flag. The sidebars would be internal so the slot would be able to be aware of them. But I agree it makes more sense to be set on the fill my only fear is it ends up being used by the plugins to have their toolbar open by default. But on second thought they can already do that using our actions.
I will try the fill approach.
The question becomes what happens if multiple fills do that
I guess the first or the last one being rendered would appear open. But it is something we need to test in practice.
a1c15cb
to
8746f6d
Compare
e7725a6
to
850f956
Compare
Description
Branched from #22140.
This PR adds the complementary area mechanism from the WordPress interface package into the widgets screen.
This allows the sidebar to be toggled on and off like in the other screens.