-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Data views table layout: Update cell vertical alignment #57804
Conversation
Size Change: +83 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
Flaky tests detected in 53fff0f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7502803466
|
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.
LGTM, thank you!
{ field.render( { | ||
item, | ||
} ) } | ||
<span className="dataviews-view-table__cell-content-wrapper"> |
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.
I now realised that we cannot have an inline element(span) as a wrapper because it creates invalid html.
The addition of styling this as display:flex
too, make the previews not render because of the width calculation.
Can we achieve what this PR does without spans and make sure the previews are displayed properly?
Sorry for not catching this earlier..
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.
We actually have an issue to remove the template previews in Table and List layouts. They're just too small to be effective in those layouts. Does that affect things here at all?
The wrapping element doesn't really matter I think... would a div
work? Or are you suggesting to remove the wrapper entirely? I couldn't think of a way to achieve the desired result (see the "After" screenshot) without one, did you have an idea?
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.
I created a follow up which will need some eyes from you :). Regarding the removal of the preview, let's handle this separately as we need to support it in general, even if we decide to remove it from these specific pages now.
What?
Align contents of table cells
top
rather thanmiddle
.Why?
Top alignment works better when there are cells that grow in height, e.g. the "Description" field in the templates table.
How?
vertical-align: top
to.dataviews-view-table tbody td
.min-height
equal to the tallest element in each row (action buttons). This enables pseuso-middle-alignment when there are no cells that grow in height.Testing Instructions
Screenshots or screencast
Note that the first 5 rows have the same appearance before and after, but in the rows with long descriptions cell contents are aligned with one another at the top of the row.