Widgets: Refactor the inspector in Widgets Customizer to use core's controls #30431
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
[Focus] Accessibility (a11y)
|
Size Change: +715 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
fix/customize-widgets-inspector-a11y
branch
from
a41c92e to
cf187c4
Compare
kevin940726
commented
Apr 1, 2021
Comment on lines
+74
to
+76
| // This is a temporary hack to prevent button component inside <BlockInspector> | ||
| // from submitting form when type="button" is not specified. | ||
| <form onSubmit={ ( event ) => event.preventDefault() }> |
There was a problem hiding this comment.
I opened ItsJonQ/g2#298 to track this.
There was a problem hiding this comment.
That PR is merged now. Can we remove the hack?
There was a problem hiding this comment.
It's not yet being merged into trunk yet.
53 tasks
|
I like this approach better 🙂 |
|
I noticed two bugs:
|
This is an oversight, fixed in c81cbab.
This is a bit weirder, it seems like it has something to do with React remounting the tree twice. Since currently I can't get 2+ block editors work nicely together at the same time on the page, I unmount the previous one whenever it's collapsed and remount it when expanded. I delay the mounting to after the transitioning to avoid the weird transitioning effect, also make it more responsive to user's interactions. If it's not solvable (rendering multiple block editors) at the time, then maybe we should consider adding some loading screen when transitioning 🤔. |
It should be possible. The idea of a |
Oh I didn't know that!
Yes we should be able to, but I think that's up to a different PR. The performance concern is crucial too. I would imagine mounting multiple fully-featured block editors has some serious performance impact, but I need to do some tests to make sure, maybe it's not that bad. |
Yeah, I see what you mean here. I am definitely not a fan of how the editor "pops in" now after the transition is complete. It makes the UI feel very unresponsive. If it is not feasible to mount the entire editor in the 16 ms that we have before the transition starts then we should try to mount only some of the block editor. Perhaps only the block editor's chrome but not the blocks. But, yes, this can be done in a follow-up 🙂 |
noisysocks
approved these changes
Apr 8, 2021
priethor
changed the title
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
Supersede #29755, close #30271.
I essentially rewrote the inspector by using core's controls section. Simplified the code a bit and moved most of the logics to
controls. This also fixes the accessibility and UX issues of the previous attempt.How has this been tested?
Screenshots
Kapture.2021-04-01.at.10.30.07.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).