Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Data views table layout: Update cell vertical alignment #57804

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Data views table layout: Update cell vertical alignment #57804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/dataviews/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,16 @@
padding-left: $grid-unit-05;
}
}

tbody {
td {
vertical-align: top;
}
.dataviews-view-table__cell-content-wrapper {
min-height: $grid-unit-40;
display: flex;
align-items: center;
}
}
.dataviews-view-table-header-button {
padding: $grid-unit-05 $grid-unit-10;
font-size: 11px;
Expand Down
34 changes: 20 additions & 14 deletions packages/dataviews/src/view-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,17 +536,21 @@ function ViewTable( {
minWidth: 20,
} }
>
<SingleSelectionCheckbox
id={ getItemId( item ) || index }
item={ item }
selection={ selection }
onSelectionChange={
onSelectionChange
}
getItemId={ getItemId }
data={ data }
primaryField={ primaryField }
/>
<span className="dataviews-view-table__cell-content-wrapper">
<SingleSelectionCheckbox
id={
getItemId( item ) || index
}
item={ item }
selection={ selection }
onSelectionChange={
onSelectionChange
}
getItemId={ getItemId }
data={ data }
primaryField={ primaryField }
/>
</span>
</td>
) }
{ visibleFields.map( ( field ) => (
Expand All @@ -560,9 +564,11 @@ function ViewTable( {
field.maxWidth || undefined,
} }
>
{ field.render( {
item,
} ) }
<span className="dataviews-view-table__cell-content-wrapper">
Copy link
Contributor

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..

Copy link
Contributor Author

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?

Copy link
Contributor

@ntsekouras ntsekouras Jan 22, 2024 •
edited
Loading

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.

{ field.render( {
item,
} ) }
</span>
</td>
) ) }
{ !! actions?.length && (
Expand Down
Loading