-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make the inserter behave as a popover #24429
Conversation
Size Change: +243 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
8b19ebc
to
8e492a7
Compare
withFocusReturn( ( { children } ) => children ) | ||
); | ||
|
||
export default function PopoverWrapper( { onClose, children } ) { |
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 was extracted as is from Popover, ideally, it should be a reusable hook/component (or multiple).
8e492a7
to
faf22d8
Compare
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.
a5d2b4b
to
fe92b1e
Compare
I think this is a great improvement, thanks Riad! Focus is constrained and predictable. Hitting Here's a gif of my test: You'll notice that the editor's content scrolls along with my position in the inserter as I tab. I'm not sure this should happen. What I find curious is that I don't see that happening on Joen's test. Maybe something funny happening on my local install? I also think screen-reader navigation would be much friendlier if we announced each group the user lands on while tabbing. Right now I only hear "Image text, Buttons text, Shortcode text", so it's hard to understand that you're on the first item of a group. I'll create a different issue as this is out of scope for this one but still wanted to note it. |
@youknowriad When you click inside a block, the main inserter closes. If you have a block with a template lock and a select list of allowed blocks, how do you add those children blocks? |
Not sure I understand what you mean here? It just works like previous versions. You click the appender of that container block and add the children blocks? |
Given this block: https://gist.github.com/ryelle/cacdba5c888153d97eae38db775dec7f (maybe there's an issue with this block?) The appender does not show up. The appender visible in the screenshot is not the containers appender. |
Actually, that's a bad example, it has a template lock. User error. Disregard. :) |
This change is quite a big regression for building pages using block patterns. I would not expect the inserter to close when I select a pattern, and I expect users would want to add multiple patterns in one go. Also, I lose my scroll position, so when reopening I have to find where I was again. I realize there will be followups to address this, I wanted to note the flow issue, especially with scroll position. |
This PR has made it incredibly difficult to add multiple blocks or patterns to a document in the post editor. It also makes it difficult to add multiple blocks or widgets to a sidebar with the widget editor. [See #26223]
I think its time we do this. I could see the same behavior for the other sidebars as well; Opening a sidebar would trap focus. Actions within the sidebar (like adding a block) can move focus away, and you could still move focus back into the sidebar — but you'd be able to use |
Hey Shaun @shaunandrews How do you suggest we move this onward? |
There's a PR that revisits this behavior over here: #28191 |
Thanks! |
closes #22858
I've been thinking about the behavior of the inserter and here's my findings so far. If we want to make the collapsible panel accessible properly It means:
That said this solution gives away the possibility to insert multiple blocks and patterns in a row. So we have two options we can explore later: