DataViews: Use button for patterns, pages and templates preview field #58071
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
ntsekouras
added
[Type] Experimental
ntsekouras
requested review from
youknowriad,
oandregal,
jameskoster and
SaxonF
|
Size Change: +444 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
It's working for me. One detail we might want to tweak though; the disabled buttons (e.g. theme patterns) have an opacity style applied on focus which is a little awkward. Could we remove that in this case? |
|
Flaky tests detected in a240d4a26bbb3ce21329d91395a46440b446c4f1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7612667176
|
jameskoster
approved these changes
Jan 22, 2024
|
I noticed that only the preview button does not have a focus style when using the tab key on the keyboard. As a result, users using only the keyboard will momentarily lose track of where the focus is. I think this issue should also be fixed in WP6.5, along with hover styles. |
|
Ah good spot, let's fix the focus styles before merging. |
|
As far as I have tested, we should be able to apply the basic focus style by making the following changes, but I may have missed something, so I would appreciate it if you could test it. diff --git a/packages/dataviews/src/style.scss b/packages/dataviews/src/style.scss
index 4cd5395895..0ae12cfd17 100644
--- a/packages/dataviews/src/style.scss
+++ b/packages/dataviews/src/style.scss
@@ -285,7 +285,6 @@
width: 100%;
min-height: 200px;
aspect-ratio: 1/1;
- overflow: hidden;
border-bottom: 1px solid $gray-200;
background-color: $gray-100;
diff --git a/packages/edit-site/src/components/page-patterns/style.scss b/packages/edit-site/src/components/page-patterns/style.scss
index 9e44fabcfe..0eb8aeeaba 100644
--- a/packages/edit-site/src/components/page-patterns/style.scss
+++ b/packages/edit-site/src/components/page-patterns/style.scss
@@ -248,10 +248,10 @@
background-color: unset;
box-sizing: border-box;
cursor: pointer;
- overflow: hidden;
+ border-radius: $radius-block-ui;
height: 100%;
- &:focus {
+ &:focus-visible {
box-shadow: inset 0 0 0 0 $white, 0 0 0 2px var(--wp-admin-theme-color);
// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
diff --git a/packages/edit-site/src/components/page-templates/style.scss b/packages/edit-site/src/components/pa
ge-templates/style.scss
index a89cab6fc8..ce90fa077a 100644
--- a/packages/edit-site/src/components/page-templates/style.scss
+++ b/packages/edit-site/src/components/page-templates/style.scss
@@ -10,10 +10,10 @@
background-color: unset;
box-sizing: border-box;
cursor: pointer;
- overflow: hidden;
+ border-radius: $radius-block-ui;
height: 100%;
- &:focus {
+ &:focus-visible {
box-shadow: inset 0 0 0 0 $white, 0 0 0 2px var(--wp-admin-theme-color);
// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
By the way, changing the preview area to a button will also solve the problem reported in #58046. |
|
I just pushed a commit similar to your suggestion. I had to add |
jameskoster
requested changes
Jan 22, 2024
There was a problem hiding this comment.
@ntsekouras I saw one other issue; in Pages I think the grey placeholder should also be a button. Same with empty patterns like @t-hamano suggests.
t-hamano
reviewed
Jan 23, 2024
There was a problem hiding this comment.
The button focus style looks good.
- When the preview is clickable, the focus outline color is the primary color (blue)
- When preview is not enabled, the focus outline color is black
- It is also visible in Windows' high contrast mode.
in Pages I think the grey placeholder should also be a button.
I've tested the grid layout in Pages, but nothing is displayed whether there is content or not. It seems to be the same with trunk. Can you reproduce this problem?
| border-bottom: 1px solid $gray-200; | ||
| background-color: $gray-100; | ||
| border-radius: 3px 3px 0 0; |
There was a problem hiding this comment.
Why is the rounded corner value 3px in this PR? How about making the corners the same as the rounded corners of the card itself?
border-radius: $radius-block-ui * 2 $radius-block-ui * 2 0 0;
There was a problem hiding this comment.
If it matches precisely then there's a small space where the background is visible between the card and the media wrapper / button. You have to zoom in to see it:
I appreciate we could avoid the additional border-radius styles if we apply overflow: hidden to the card, but then the button focus style would be hidden.
If you have another idea how to tackle this please let me know, or feel free to push to the branch :)
There was a problem hiding this comment.
Thank you for your detailed explanation! I can't think of any good ideas for now, so Iet's keep these styles 😅
ntsekouras
force-pushed
the
add/link-to-dataviews-preview
branch
from
3488050
to
2f5b1e9
Compare
|
Thank you for your help here @t-hamano and @jameskoster! 🙏 I've pushed some updates to always render a button, even if a media item or preview is empty. |
|
@ntsekouras thanks for that change. I think the button should still link to edit view even when there's no preview. Currently it is clickable but nothing happens. Apologies I should have been more explicit with that request. I suspect that change would fix the focus style discrepancy @t-hamano pointed out. |
|
I pushed a fix for the Pages / Templates views. For patterns we should only render the button for editable patterns. The link behavior should basically match the title – if the title is a link then the preview should be too. Sorry for the confusion! |
ntsekouras
force-pushed
the
add/link-to-dataviews-preview
branch
from
37b6a54
to
9a56869
Compare
I pushed the update. I think this is probably ready to land right now.. |
ntsekouras
force-pushed
the
add/link-to-dataviews-preview
branch
from
c820463
to
9f58d51
Compare
ntsekouras
changed the title
jameskoster
approved these changes
Jan 23, 2024
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?
Fixes: #58046
This PR adds a button wrapper to patterns' and templates' previews to link to the entity. This is an adhoc solution in order to have this functionality for now and explore a possible alternative API to provide a 'main/edit' link or something similar that could be used in a generic way in other places too, if needed.
Testing Instructions
My patternssections are actually linked, so you have to test the clickable preview there.Notes
tableview and there is a possible fix here.pages?Screenshots or screencast
Screen.Recording.2024-01-22.at.1.21.23.PM.mov