Pattern Inserter: show insertion indicator when hovering on patterns #47316
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
t-hamano
added
[Feature] Inserter
|
Size Change: +4.76 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 171b896. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3986677113
|
|
I am working on fixing the E2E test. |
|
I wonder if we should be showing this line "on focus" personally, it seems weird to show this directly as you open the pattern sidebar. |
Selecting a pattern category automatically focuses on the first pattern in that category. Thus, the inserter is displayed. |
|
No, I mean that we only show the indicator on hover, not on focus. The focus on open is necessary for a11y reasons. |
I see, I updated it that way. I have made the same change for the regular block, should this be submitted as a separate PR? The following is the behavior achieved by this PR: 467a5bba646c817c33803ea2df25b166.mp4 |
|
This makes sense to be but it would be cool to confirm with others: (only show the indicator on hover). cc @jasmussen @jameskoster |
|
It seems useful on hover, yes, and helps contextualize the action. We can always revisit if it feels useful beyond that. |
|
To clarify this PR removes the indicator when a block is focused on the inserter as well. |
t-hamano
commented
Jan 26, 2023
Comment on lines
-350
to
+353
| await page.waitForSelector( '.editor-block-list-item-paragraph' ); | ||
| await page.focus( '.editor-block-list-item-paragraph' ); | ||
| const paragraphButton = ( | ||
| await page.$x( `//button//span[contains(text(), 'Paragraph')]` ) | ||
| )[ 0 ]; | ||
| await paragraphButton.hover(); |
There was a problem hiding this comment.
Fixed one E2E test failure.
In this PR, hover() must be performed because the inserter is no longer visible when in focus().
|
We can see how this feels in practice. In principle it sounds good to show the indicator on focus, but it isn't entirely clear. So seems good to try. |
|
I think we have slight regression after this PR. When I hover over a Pattern, I see a block preview modal behind the Screenshot
|
15 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.
Closes: #45183
What?
This PR shows the insertion indicator when hovering over a pattern in the main pattern inserter dialog.
Why?
The main inserter currently has four tabs: Blocks, Patterms, Media, Reusable Block. Of these, for Patterns and Media, the indicator doesn't appear when hovering over an item. I believe it is necessary from a usability standpoint to indicate where an item will be inserted, no matter what kind of item it is.
How?
The specific implementation was based on what was already implemented in the block. One difference is that I used state instead of ref to manage whether or not we are dragging.
Testing Instructions
In the main pattern inserter, check the following:
Screenshots or screencast
846f14d508ec10b79e4e6c8b74a74c7d.mp4
Next Step
If this PR makes sense, I would like to add a similar implementation in the media dialog in another PR.