Add customize widgets inserter #29549
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
kevin940726
added
the
[Feature] Widgets Customizer
|
Size Change: +2.18 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
add/customize-widgets-inserter
branch
from
c02051e
to
21fae3c
Compare
noisysocks
requested changes
Mar 15, 2021
There was a problem hiding this comment.
Really nice work here! It's looking fantastic.
I noticed three bugs while testing:
-
In Firefox on macOS, the Quick Inserter appears underneath the Customizer's scroll bar.
-
When you open the Customizer's publish options, the inserter does not close.
Kapture.2021-03-15.at.14.26.53.mp4
-
When you navigate away from Widgets, the inserter does not close.
Kapture.2021-03-15.at.14.29.37.mp4
I'd also like to explore if we can re-use the Customizer's outer-section-open instead of re-implementing our own slide out and fade animation.
This is not the behaviour for the existing slide-out panel that the Customizer has, so I say no. |
11 tasks
|
I think we should try and match the behavior of the other panes that hide/appear when using the Customizer, like the publish options or the menu's inserter. Looking at the existing panes they...
My instinct tells me that we should allow for clicking the scrim to close the pane, but I don't think we should venture down that road unless we're willing to add/change the behavior everywhere else in the Customizer. Similarly, I'd expect the color of the scrim and the transition timing function to match the existing Customizer panes. -- One thing that stood out to me was that |
kevin940726
force-pushed
the
add/customize-widgets-inserter
branch
from
02c7926
to
b9da7c8
Compare
noisysocks
reviewed
Mar 23, 2021
packages/customize-widgets/src/components/inserter/use-inserter.js
Outdated
Show resolved
Hide resolved
packages/customize-widgets/src/components/inserter/use-inserter.js
Outdated
Show resolved
Hide resolved
|
Amazing work! I'm honestly impressed at how smooth you've gotten this working. It all works really well when I run it locally. Instead of having So, instead of having a class SidebarControl extends customize.Control {
...
render() {
if ( this.expanded() ) {
render(
<SidebarBlockEditor
sidebar={ new SidebarAdapter( this.setting, customize ) }
openInserter={ this.openInserter.bind( this ) }
closeInserter={ this.closeInserter.bind( this ) }
/>,
this.container[ 0 ]
);
} else {
...
}
}
openInserter() {
// insert stateful Backbone & DOM stuff here
}
closeInserter() {
// insert stateful Backbone & DOM stuff here
}
}I think this may be simpler to understand and might make it easier to merge these changes into Core. What do you think? |
|
@noisysocks That's a great suggestion! However, I think we would eventually need to access the state in React anyway. There's already |
|
Ah, right, I suppose you could pass down an object which has We could even use an I think that there's value in trying to keep these two different ways of building applications (Backbone / React) from mixing with each other and then, where it is necessary for them to mix, using explicit interfaces (like |
noisysocks
reviewed
Mar 26, 2021
packages/customize-widgets/src/components/inserter/inserter-outer-section.js
Outdated
Show resolved
Hide resolved
noisysocks
approved these changes
Mar 26, 2021
Co-authored-by: Robert Anderson <robert@noisysocks.com>
kevin940726
force-pushed
the
add/customize-widgets-inserter
branch
from
4e0abae
to
38a8fd1
Compare
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
Close #29285.
Known issues
Clicking on the backdrop of the inserter won't close it (should it?)(it shouldn't)How has this been tested?
Screenshots
Kapture.2021-03-23.at.01.58.14.mp4
Types of changes
New feature
Checklist: