DataViews: Fix nested buttons and placeholder text in list layout #58304
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
[Type] Bug
|
Size Change: +22 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
t-hamano
force-pushed
the
fix/dataviews-nested-button-and-placeholder
branch
from
47d27c3 to
5d20ce1
Compare
ntsekouras
reviewed
Jan 29, 2024
| </button> | ||
| { viewType === LAYOUT_LIST && hasMedia && ( | ||
| <Media | ||
| className="edit-site-page-pages__featured-image" |
There was a problem hiding this comment.
Nit: I guess we could have Media in a constant above as it's identical in both scenarios
ntsekouras
reviewed
Jan 29, 2024
| { ! isEmpty && <BlockPreview blocks={ blocks } /> } | ||
| </button> | ||
| { viewType === LAYOUT_LIST ? ( | ||
| <BlockPreview blocks={ blocks } /> |
There was a problem hiding this comment.
Nit: we could also check for isEmpty before rendering.
ntsekouras
approved these changes
Jan 29, 2024
|
@ntsekouras Thanks for the review! I have updated it with fba7764, but could you confirm that your feedback is reflected correctly in the code? |
Yeah, looks good. Thank you! |
|
Flaky tests detected in fba7764b00e8bc16a8e732f50dc738d9fa7d7644. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7695314918
|
ntsekouras
force-pushed
the
fix/dataviews-nested-button-and-placeholder
branch
from
fba7764 to
4cbd8a7
Compare
I wonder if there's an icon-only treatment that works in all layouts we might try. Perhaps one to look into in a follow-up? |
I think this is a good idea and may be able to address it in a follow-up. However, even if I search the icon library, there might not be an icon to represent "empty" or "placeholder" yet 🤔 |
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.
Related to #58071
What?
This PR solves the following two problems with Templates and Pages list layout.
Preview consists of nested buttons
Placeholder text overflows
Why?
Preview consists of nested buttons
When in the list layout, the entire row has a button role. Therefore, the preview inside it should not have been a button element.
Placeholder text overflows
Placeholder text works fine in the grid layout, but in the list layout, the media width is only 32px, so the text cannot be displayed properly.
How?
###Preview consists of nested buttons
When in the list view, unwrap the button element.
Placeholder text overflows
When in the list view, the placeholder text itself is not displayed.
Testing Instructions
Pages
Templates