Inserter: Add close button #61421
Merged
Inserter: Add close button #61421
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
scruffian
commented
May 7, 2024
| @@ -24,6 +24,7 @@ function InserterLibrary( | |||
| __experimentalOnPatternCategorySelection, | |||
| onSelect = noop, | |||
| shouldFocusBlock = false, | |||
| onClose, | |||
There was a problem hiding this comment.
It's not great to pass the props here, but I can't find a way to access this state without passing it down.
There was a problem hiding this comment.
This will get undone when we refactor the List View and Inserter to share the same sidebar, so I think it's OK.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ntsekouras
reviewed
May 7, 2024
| <Button | ||
| className="editor-inserter-sidebar__close-button" | ||
| icon={ closeSmall } | ||
| label={ __( 'Close block inserter' ) } |
There was a problem hiding this comment.
Should the label always be block inserter? Should we have a generic label for the inserter or update per selected tab?
There was a problem hiding this comment.
Makes sense to update it, in a different PR :)
ntsekouras
reviewed
May 7, 2024
|
Size Change: +7 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in c002a32. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8980362927
|
|
Closing in favour of #61426 |
MaggieCabrera
added a commit
that referenced
this pull request
May 8, 2024
Merged
scruffian
force-pushed
the
update/inserter-close-button
branch
2 times, most recently
from
7092ac1
to
eb6ba04
Compare
scruffian
force-pushed
the
update/inserter-close-button
branch
from
eb6ba04
to
7ea9ac2
Compare
jeryj
approved these changes
May 8, 2024
| @@ -24,6 +24,7 @@ function InserterLibrary( | |||
| __experimentalOnPatternCategorySelection, | |||
| onSelect = noop, | |||
| shouldFocusBlock = false, | |||
| onClose, | |||
There was a problem hiding this comment.
This will get undone when we refactor the List View and Inserter to share the same sidebar, so I think it's OK.
|
I am unable to access this button with the keyboard, is there a trick to it? |
|
@carolinan you can't access the button because it's actually placed before the tabs but it appears after them. That's extremely confusing as the visual roder doesn't match the DOM order. We then agreed to close that PR in favor of a more general solution in #59013 but that issue didn't get the attention and priority it deserves. In the meantime, this PR introduces one more instance of the problem. I'm not sure proceeding this way helps building a more usable and accessible user interface in any way. Trying to stay positive, I'd like to encourage everyone to participate to #59013 to help find a new design to solve the underlying issue, following the two base principles explained in #59013 (comment) |
afercia
added
the
[Focus] Accessibility (a11y)
This was referenced May 8, 2024
|
Thanks both, the aim is to move towards a common approach for ListView and the Inserter. Ultimately we should be using the same code for both, so improvements to the ListView will also benefit this area. |
To answer your specific question the trick is Shift + Tab |
|
Shift + Tab is used to move focus to the previous item, it is not intuitive or discoverable that you have to move backwards to select an item that is visually positioned after the currently focused item. |
This si definitely not the expected keyboard interaction. As said earlier, this change actually introduced one more occurrence of the bug that is now tracked at #59013 |
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.
What?
Add a close button to the inserter.
Why?
We'd like to leave the inserter always open, which means it needs a close button - see #60391.
How?
Copies the implementation from the List View
Testing Instructions
Screenshots or scree