Focus control in widgets customizer #31308
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
[Type] Bug
|
Size Change: +597 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
talldan
reviewed
Apr 30, 2021
There was a problem hiding this comment.
This works really well @kevin940726.
Only thing I noticed is that when moving a block to a new widget area, the block isn't always focused. I think we can still move forwards with merging and address that after. Clicking the pencil icons is very slick though!
Code structure looks like a nice improvement—at some point it'd be good to follow-up with documentation.
Should we ship this?
talldan
approved these changes
Apr 30, 2021
Huh, I can't reproduce this! Can you reliably reproduce it? Would appreciate if you can open an issue or leave a comment here 🙏
I'm still working on adding some e2e tests. Then it should be ready for review (and merge). |
kevin940726
requested review from
ajitbohra,
nerrad,
ntwb,
spacedmonkey and
TimothyBJacobs
as code owners
I pushed some changes to try to fix that. Please help review it again 🙏 |
|
Still looks good to me, thanks for adding tests. |
kevin940726
commented
May 3, 2021
Comment on lines
+46
to
+47
| this.setting.bind( this._handleSettingChange.bind( this ) ); | ||
| this.api.bind( 'change', this._handleAllSettingsChange.bind( this ) ); |
There was a problem hiding this comment.
Move to constructor so that it can still listen to the changes even when it's not active yet.
noisysocks
approved these changes
May 4, 2021
|
There are a few conflicts now after #31405 was merged, |
kevin940726
force-pushed
the
fix/widgets-customizer-focus-control
branch
from
67cc663 to
76cf4e8
Compare
kevin940726
force-pushed
the
fix/widgets-customizer-focus-control
branch
from
a0ba1d1 to
3d52629
Compare
7 tasks
|
Nice work! |
7 tasks
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 #30897.
Handle focus control in the Widgets Customizer. We listen to the
focus-control-for-settingevent globally and handle expanding the section and focusing on the block.I also revamped the structure a little bit, instead of mounting and unmounting a block editor React tree for each sidebar, we mount one React tree globally (virtually) and use
createPortalto handle rendering the actual editor to each sidebar. We still have to unmount and remount the editor when switching to another sidebar though, the performance does not improve much. However, I hope that the code is bit more structured and flexible now. One benefit of this approach is that we can now use React context to pass down states down the tree.<MoveToSidebar>can now be written in a more React-way (if that's a thing).Also upgrade
puppeteer-testing-libraryto v0.5.0 (full changelog). This enable us to wait fortoHaveFocus()to finish.How has this been tested?
Added tests in
customizing-widgets.Screenshots
Kapture.2021-04-29.at.14.11.49.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).