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

DataViews: Fix table view cell wrapper and BlockPreviews #58062

Merged
merged 2 commits into from
Jan 26, 2024
Merged

DataViews: Fix table view cell wrapper and BlockPreviews #58062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a5347a
DataViews: Fix table view cell wrapper and BlockPreviews
ntsekouras Jan 22, 2024
af67ba2
try preserving flex
ntsekouras Jan 22, 2024
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
4 changes: 4 additions & 0 deletions packages/dataviews/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@
min-height: $grid-unit-40;
display: flex;
align-items: center;

> * {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameskoster I gave it a try to be able to render block previews with this, but I'm not 100% sure if it's the right solution.

In general the previews issue stems from useResizeObserver, where it seems it can determine a width if we have just the display:flex. I'm not sure if @ciampo has any thoughts on this about whether it's something that should be handled by the hook.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nik. Visually this looks a lot better. Subject to Marco's feedback we can probably add a fixed height to the previews in table layout so they are consistent. It would also be nice to apply the theme background color to the container to avoid these awkward gray spaces in grid layout:

grey

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subject to Marco's feedback we can probably add a fixed height to the previews in table layout so they are consistent.

It already looks like we crop template previews to a max-height in the table view, so we could easily lower that number — although that means that if a template preview image is shorter than that image, that row will take less space.

If instead we want to enforce a fixed height, we could adopt the same technique employed for the Grid layout (basically the object-fit: cover behavior).

In any case, this could be discussed / applied in a separate PR, since the goal of this PR is to fix the markup and make sure that the preview image appears correctly.

It would also be nice to apply the theme background color to the container to avoid these awkward gray spaces in grid layout

I hacked around and managed to achieve that with this code 
diff --git a/packages/dataviews/src/view-grid.js b/packages/dataviews/src/view-grid.js
index c8cc3516e1..16d39dac3a 100644
--- a/packages/dataviews/src/view-grid.js
+++ b/packages/dataviews/src/view-grid.js
@@ -1,6 +1,7 @@
 /**
  * WordPress dependencies
  */
+import { privateApis as blockEditorPrivateApis } from '@wordpress/block-editor';
 import {
 	__experimentalGrid as Grid,
 	__experimentalHStack as HStack,
@@ -13,6 +14,9 @@ import { useAsyncList } from '@wordpress/compose';
  * Internal dependencies
  */
 import ItemActions from './item-actions';
+import { unlock } from './lock-unlock';
+
+const { useGlobalStyle } = unlock( blockEditorPrivateApis );
 
 export default function ViewGrid( {
 	data,
@@ -22,6 +26,8 @@ export default function ViewGrid( {
 	getItemId,
 	deferredRendering,
 } ) {
+	const [ backgroundColor ] = useGlobalStyle( 'color.background' );
+
 	const mediaField = fields.find(
 		( field ) => field.id === view.layout.mediaField
 	);
@@ -50,7 +56,10 @@ export default function ViewGrid( {
 					key={ getItemId( item ) }
 					className="dataviews-view-grid__card"
 				>
-					<div className="dataviews-view-grid__media">
+					<div
+						className="dataviews-view-grid__media"
+						style={ { backgroundColor } }
+					>
 						{ mediaField?.render( { item } ) }
 					</div>
 					<HStack

Although we should probably expose an API and apply a background color only when we are certain that the media that we are previewing would actually match that background color (like when managing templates).

This is also something that we should probably experiment with in a separate PR.

Finally, I wanted to flag that, when I pick the Grid layout and reload the page, the template preview images didn't load. In order to have them load correctly, I had to switch to the Table layout, and enable the preview field.

flex-grow: 1;
}
}
}
.dataviews-view-table-header-button {
Expand Down
8 changes: 4 additions & 4 deletions packages/dataviews/src/view-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ function ViewTable( {
minWidth: 20,
} }
>
<span className="dataviews-view-table__cell-content-wrapper">
<div className="dataviews-view-table__cell-content-wrapper">
<SingleSelectionCheckbox
id={
getItemId( item ) || index
Expand All @@ -553,7 +553,7 @@ function ViewTable( {
data={ data }
primaryField={ primaryField }
/>
</span>
</div>
</td>
) }
{ visibleFields.map( ( field ) => (
Expand All @@ -567,7 +567,7 @@ function ViewTable( {
field.maxWidth || undefined,
} }
>
<span
<div
className={ classnames(
'dataviews-view-table__cell-content-wrapper',
{
Expand All @@ -580,7 +580,7 @@ function ViewTable( {
{ field.render( {
item,
} ) }
</span>
</div>
</td>
) ) }
{ !! actions?.length && (
Expand Down
Loading