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.

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

DataViews: Implement isItemClickable and onClickItem props #66365

Merged
merged 75 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 61 commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
785085b
Page Edit View: Implement Featured image page field
gigitux Aug 14, 2024
dd7e0a3
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Aug 14, 2024
77f2217
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Aug 21, 2024
433e45f
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Aug 21, 2024
f890963
improve flow and style
gigitux Aug 22, 2024
c32622e
fix list layout
gigitux Aug 22, 2024
9a39fe6
improve design
gigitux Aug 23, 2024
5419b0a
remove not necessary style
gigitux Aug 23, 2024
68ef300
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Aug 26, 2024
cd923f8
improve style
gigitux Aug 27, 2024
94335a4
improve default image control
gigitux Aug 27, 2024
030a28d
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Aug 27, 2024
baf7a82
fix style
gigitux Aug 29, 2024
4095dab
remove not necessary configuration
gigitux Aug 29, 2024
23d8695
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Aug 29, 2024
ec715ed
format _z-index.scss file
gigitux Aug 29, 2024
ea29690
improve style
gigitux Aug 29, 2024
106af4d
remove not necessary code
gigitux Aug 29, 2024
cc46724
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Aug 29, 2024
b11e923
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Sep 9, 2024
c6dffd7
fix focus
gigitux Sep 9, 2024
3c262d6
remove image field type
gigitux Sep 9, 2024
c053a9a
add comment
gigitux Sep 9, 2024
57faa8f
fix warning
gigitux Sep 9, 2024
d2eeb0e
fix image for deleted pages
gigitux Sep 9, 2024
05f0d1a
remove filename
gigitux Sep 10, 2024
c4db3b9
add border-radius
gigitux Sep 10, 2024
cd9fe2f
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Sep 18, 2024
8356977
migrate featured image to fields package
gigitux Sep 18, 2024
e27dec1
fix CSS
gigitux Sep 19, 2024
a187591
remove not necessary changes
gigitux Sep 19, 2024
8815e5a
fix type
gigitux Sep 19, 2024
a461d85
remove empty space
gigitux Sep 19, 2024
fcc2e6e
fix z-index gallery
gigitux Sep 19, 2024
577f836
fix overlapping and style
gigitux Sep 19, 2024
a3449c4
improve codestyle
gigitux Sep 19, 2024
2366429
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Sep 20, 2024
1b84ac0
use same placeholder style grid/list layout
gigitux Sep 20, 2024
6ca01ff
fix tsconfig
gigitux Sep 20, 2024
8e612d4
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Oct 22, 2024
c448c49
add type
gigitux Oct 22, 2024
2b595c2
remove mediaField from layout table
gigitux Oct 22, 2024
9cc6d80
update type
gigitux Oct 22, 2024
01a57a8
use text as type
gigitux Oct 22, 2024
7b1e40a
use media-utils package
gigitux Oct 22, 2024
6592d36
fix tsconfig
gigitux Oct 22, 2024
c2994e0
remove view prop
gigitux Oct 22, 2024
b039310
revert changes
gigitux Oct 22, 2024
42f90f4
generate package-lock.json
gigitux Oct 22, 2024
1c3bdb9
add onClick and isClickable props
gigitux Oct 23, 2024
ea8e941
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/page-…
gigitux Oct 23, 2024
1d0809a
generate package-lock.json
gigitux Oct 23, 2024
e6e3eb9
add documentation
gigitux Oct 23, 2024
09b2f79
fix naming
gigitux Oct 23, 2024
9e2cbf9
improve documentation
gigitux Oct 23, 2024
08ea4d5
Merge branch 'add/page-image-control' of github.com:gigitux/gutenberg…
gigitux Oct 23, 2024
e77b93b
generate package-lock.json
gigitux Oct 23, 2024
5bf9726
Merge branch 'add/page-image-control' of github.com:WordPress/gutenbe…
gigitux Oct 23, 2024
a8e3d75
update documentation
gigitux Oct 24, 2024
c9ab19d
fix prop name
gigitux Oct 30, 2024
9f7c57d
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/selec…
gigitux Nov 4, 2024
eb2a816
fix logic table view
gigitux Nov 4, 2024
f8d86b1
Rename isClickable to isItemClickable and onClick to onItemClick for …
gigitux Nov 4, 2024
5c7b0c4
fix style
gigitux Nov 4, 2024
f8036fd
Merge branch 'trunk' of github.com:WordPress/gutenberg into add/selec…
gigitux Nov 5, 2024
419f3fa
rename onItemClick to onClickItem
gigitux Nov 5, 2024
7a79644
improve CSS
gigitux Nov 5, 2024
0071e09
fix docs
gigitux Nov 5, 2024
507d60b
add useClickableItemProps
gigitux Nov 5, 2024
ed48f39
use `isClickable` variable
gigitux Nov 6, 2024
c9e418b
improve code style
gigitux Nov 6, 2024
fdc2892
Address feedback about getClickableItemProps
oandregal Nov 12, 2024
84cc4c2
Remove defaults, they are already provided by the Provider
oandregal Nov 12, 2024
eaa3324
Cache empty actions
oandregal Nov 12, 2024
f24172b
Address feedback: simplify getClickableItemProps
oandregal Nov 12, 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
10 changes: 9 additions & 1 deletion packages/dataviews/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,15 @@ The `defaultLayouts` property should be an object that includes properties named

### `onChangeSelection`: `function`

Callback that signals the user selected one of more items, and takes them as parameter. So far, only the `list` view implements it.
Callback that signals the user selected one of more items, and takes them as parameter.

### `isClickable`: `function`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about the naming.


A function that determines if a media field or a primary field are clickable. It receives an item as an argument and returns a boolean value indicating whether the item can be clicked.

### `onClick`: `function`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here.


A callback function that is triggered when a user clicks on a media field or primary field. This function is currently implemented only in the `grid` and `list` views.

## Types

Expand Down
4 changes: 4 additions & 0 deletions packages/dataviews/src/components/dataviews-context/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ type DataViewsContextType< Item > = {
openedFilter: string | null;
setOpenedFilter: ( openedFilter: string | null ) => void;
getItemId: ( item: Item ) => string;
onClick: ( item: Item ) => void;
isClickable: ( item: Item ) => boolean;
density: number;
};

Expand All @@ -43,6 +45,8 @@ const DataViewsContext = createContext< DataViewsContextType< any > >( {
setOpenedFilter: () => {},
openedFilter: null,
getItemId: ( item ) => item.id,
onClick: () => {},
isClickable: () => false,
density: 0,
} );

Expand Down
4 changes: 4 additions & 0 deletions packages/dataviews/src/components/dataviews-layout/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export default function DataViewsLayout() {
onChangeSelection,
setOpenedFilter,
density,
onClick,
isClickable,
} = useContext( DataViewsContext );

const ViewComponent = VIEW_LAYOUTS.find( ( v ) => v.type === view.type )
Expand All @@ -44,6 +46,8 @@ export default function DataViewsLayout() {
onChangeSelection={ onChangeSelection }
selection={ selection }
setOpenedFilter={ setOpenedFilter }
onClick={ onClick }
isClickable={ isClickable }
view={ view }
density={ density }
/>
Expand Down
6 changes: 6 additions & 0 deletions packages/dataviews/src/components/dataviews/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ type DataViewsProps< Item > = {
defaultLayouts: SupportedLayouts;
selection?: string[];
onChangeSelection?: ( items: string[] ) => void;
onClick?: ( item: Item ) => void;
isClickable?: ( item: Item ) => boolean;
header?: ReactNode;
} & ( Item extends ItemWithId
? { getItemId?: ( item: Item ) => string }
Expand All @@ -65,6 +67,8 @@ export default function DataViews< Item >( {
defaultLayouts,
selection: selectionProperty,
onChangeSelection,
onClick,
isClickable,
header,
}: DataViewsProps< Item > ) {
const [ selectionState, setSelectionState ] = useState< string[] >( [] );
Expand Down Expand Up @@ -110,6 +114,8 @@ export default function DataViews< Item >( {
openedFilter,
setOpenedFilter,
getItemId,
isClickable: isClickable ?? ( () => false ),
onClick: onClick ?? ( () => {} ),
density,
} }
>
Expand Down
53 changes: 51 additions & 2 deletions packages/dataviews/src/dataviews-layouts/grid/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ interface GridItemProps< Item > {
selection: string[];
onChangeSelection: SetSelection;
getItemId: ( item: Item ) => string;
onClick: ( item: Item ) => void;
isClickable: ( item: Item ) => boolean;
item: Item;
actions: Action< Item >[];
mediaField?: NormalizedField< Item >;
Expand All @@ -41,6 +43,8 @@ interface GridItemProps< Item > {
function GridItem< Item >( {
selection,
onChangeSelection,
onClick,
isClickable,
getItemId,
item,
actions,
Expand All @@ -59,6 +63,7 @@ function GridItem< Item >( {
const renderedPrimaryField = primaryField?.render ? (
<primaryField.render item={ item } />
) : null;

return (
<VStack
spacing={ 0 }
Expand All @@ -81,7 +86,27 @@ function GridItem< Item >( {
}
} }
>
<div className="dataviews-view-grid__media">
<div
className={ clsx( 'dataviews-view-grid__media', {
'dataviews-view-grid__media--clickable':
isClickable( item ),
} ) }
tabIndex={ isClickable( item ) ? 0 : undefined }
role="button"
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
onClick={ () => {
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
if ( isClickable( item ) ) {
onClick( item );
}
} }
onKeyDown={ ( event ) => {
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
if (
( event.key === 'Enter' || event.key === '' ) &&
isClickable( item )
) {
onClick( item );
}
} }
>
{ renderedMediaField }
</div>
<SingleSelectionCheckbox
Expand All @@ -96,7 +121,27 @@ function GridItem< Item >( {
justify="space-between"
className="dataviews-view-grid__title-actions"
>
<HStack className="dataviews-view-grid__primary-field">
<HStack
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of following the same approach as the table layout? This is: adding an extra div for the content, so the click target is the title text instead of the "cell area":

Screen.Recording.2024-11-04.at.18.29.09.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with 7a79644:

Screen.Capture.on.2024-11-05.at.17-00-27.mov

className={ clsx( 'dataviews-view-grid__primary-field', {
'dataviews-view-grid__primary-field--clickable':
isClickable( item ),
} ) }
tabIndex={ isClickable( item ) ? 0 : undefined }
role="button"
onClick={ () => {
if ( isClickable( item ) ) {
onClick( item );
}
} }
onKeyDown={ ( event ) => {
if (
( event.key === 'Enter' || event.key === '' ) &&
isClickable( item )
) {
onClick( item );
}
} }
>
{ renderedPrimaryField }
</HStack>
<ItemActions item={ item } actions={ actions } isCompact />
Expand Down Expand Up @@ -170,6 +215,8 @@ export default function ViewGrid< Item >( {
getItemId,
isLoading,
onChangeSelection,
onClick,
isClickable,
selection,
view,
density,
Expand Down Expand Up @@ -223,6 +270,8 @@ export default function ViewGrid< Item >( {
key={ getItemId( item ) }
selection={ selection }
onChangeSelection={ onChangeSelection }
onClick={ onClick }
isClickable={ isClickable }
getItemId={ getItemId }
item={ item }
actions={ actions }
Expand Down
9 changes: 9 additions & 0 deletions packages/dataviews/src/dataviews-layouts/grid/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@

.dataviews-view-grid__primary-field {
min-height: $grid-unit-40; // Preserve layout when there is no ellipsis button

&--clickable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is &--clickable the best way to achieve this? We don't seem to do this in Gutenberg (we have only one occurrence of &--is-hex). What does this do that couldn't be achieved by targeting &[role="button"]?

Copy link
Member

Choose a reason for hiding this comment

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

I agree the way this handles the classes needs to be iterated, see related conversation #66365 (comment)

In discussing with Riad IRL the idea was to land this PR and iterate.

cursor: pointer;
}
}


&.is-selected {
.dataviews-view-grid__fields .dataviews-view-grid__field .dataviews-view-grid__field-value {
color: $gray-900;
Expand Down Expand Up @@ -56,6 +61,10 @@
border-radius: $grid-unit-05;
pointer-events: none;
}

&--clickable {
cursor: pointer;
}
}

.dataviews-view-grid__fields {
Expand Down
35 changes: 35 additions & 0 deletions packages/dataviews/src/dataviews-layouts/table/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ interface TableColumnFieldProps< Item > {
primaryField?: NormalizedField< Item >;
field: NormalizedField< Item >;
item: Item;
isClickable: ( item: Item ) => boolean;
onClick: ( item: Item ) => void;
}

interface TableColumnCombinedProps< Item > {
Expand All @@ -48,6 +50,8 @@ interface TableColumnCombinedProps< Item > {
field: CombinedField;
item: Item;
view: ViewTableType;
isClickable: ( item: Item ) => boolean;
onClick: ( item: Item ) => void;
}

interface TableColumnProps< Item > {
Expand All @@ -56,6 +60,8 @@ interface TableColumnProps< Item > {
item: Item;
column: string;
view: ViewTableType;
isClickable: ( item: Item ) => boolean;
onClick: ( item: Item ) => void;
}

interface TableRowProps< Item > {
Expand All @@ -69,6 +75,8 @@ interface TableRowProps< Item > {
selection: string[];
getItemId: ( item: Item ) => string;
onChangeSelection: SetSelection;
isClickable: ( item: Item ) => boolean;
onClick: ( item: Item ) => void;
}

function TableColumn< Item >( {
Expand Down Expand Up @@ -102,13 +110,32 @@ function TableColumnField< Item >( {
primaryField,
item,
field,
isClickable,
onClick,
}: TableColumnFieldProps< Item > ) {
return (
<div
className={ clsx( 'dataviews-view-table__cell-content-wrapper', {
'dataviews-view-table__primary-field':
primaryField?.id === field.id,
'dataviews-view-table__primary-field--clickable':
isClickable( item ),
} ) }
tabIndex={ isClickable( item ) ? 0 : undefined }
role="button"
onClick={ () => {
if ( isClickable( item ) ) {
onClick( item );
}
} }
onKeyDown={ ( event ) => {
if (
( event.key === 'Enter' || event.key === '' ) &&
isClickable( item )
) {
onClick( item );
}
} }
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks how the onChangeSelection works currently and causes onClick to be triggered for every column except for the selection column.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be only added for the primary field? ( primaryField?.id === field.id ? )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that! I fixed the issue:

Screen.Capture.on.2024-11-04.at.14-35-39.mp4

I wrapped the primary field in a dedicated div to ensure onItemClick is invoked only when the user clicks directly on the field.

<div
className={ clsx( 'dataviews-view-table__cell-content', {
'dataviews-view-table__cell-content--clickable':
isItemClickableField,
} ) }
tabIndex={ isItemClickableField ? 0 : undefined }
role="button"
onClick={ () => {
if ( isItemClickableField ) {
onItemClick( item );
}
} }
onKeyDown={ ( event ) => {
if (
( event.key === 'Enter' || event.key === '' ) &&
isItemClickableField
) {
onItemClick( item );
}
} }
>

>
<field.render { ...{ item } } />
</div>
Expand Down Expand Up @@ -139,6 +166,8 @@ function TableRow< Item >( {
primaryField,
selection,
getItemId,
isClickable,
onClick,
onChangeSelection,
}: TableRowProps< Item > ) {
const hasPossibleBulkAction = useHasAPossibleBulkAction( actions, item );
Expand Down Expand Up @@ -214,6 +243,8 @@ function TableRow< Item >( {
<td key={ column } style={ { width, maxWidth, minWidth } }>
<TableColumn
primaryField={ primaryField }
isClickable={ isClickable }
onClick={ onClick }
fields={ fields }
item={ item }
column={ column }
Expand Down Expand Up @@ -252,6 +283,8 @@ function ViewTable< Item >( {
onChangeSelection,
selection,
setOpenedFilter,
onClick,
isClickable,
view,
}: ViewTableProps< Item > ) {
const headerMenuRefs = useRef<
Expand Down Expand Up @@ -392,6 +425,8 @@ function ViewTable< Item >( {
selection={ selection }
getItemId={ getItemId }
onChangeSelection={ onChangeSelection }
onClick={ onClick }
isClickable={ isClickable }
/>
) ) }
</tbody>
Expand Down
2 changes: 2 additions & 0 deletions packages/dataviews/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,8 @@ export interface ViewBaseProps< Item > {
onChangeSelection: SetSelection;
selection: string[];
setOpenedFilter: ( fieldId: string ) => void;
onClick: ( item: Item ) => void;
isClickable: ( item: Item ) => boolean;
view: View;
density: number;
}
Expand Down
36 changes: 7 additions & 29 deletions packages/edit-site/src/components/post-fields/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,7 @@ import { useEntityRecords, store as coreStore } from '@wordpress/core-data';
/**
* Internal dependencies
*/
import {
LAYOUT_GRID,
LAYOUT_TABLE,
OPERATOR_IS_ANY,
} from '../../utils/constants';
import { default as Link } from '../routes/link';
import { OPERATOR_IS_ANY } from '../../utils/constants';

// See https://github.com/WordPress/gutenberg/issues/55886
// We do not support custom statutes at the moment.
Expand Down Expand Up @@ -139,7 +134,7 @@ function PostAuthorField( { item } ) {
);
}

function usePostFields( viewType ) {
function usePostFields() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice change, it means we can probably move all these fields into the fields package from now on.

const { records: authors, isResolving: isLoadingAuthors } =
useEntityRecords( 'root', 'user', { per_page: -1 } );

Expand All @@ -164,30 +159,10 @@ function usePostFields( viewType ) {
? item.title
: item.title?.raw,
render: ( { item } ) => {
const addLink =
[ LAYOUT_TABLE, LAYOUT_GRID ].includes( viewType ) &&
item.status !== 'trash';
const renderedTitle =
typeof item.title === 'string'
? item.title
: item.title?.rendered;
const title = addLink ? (
<Link
params={ {
postId: item.id,
postType: item.type,
canvas: 'edit',
} }
>
{ decodeEntities( renderedTitle ) ||
__( '(no title)' ) }
</Link>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to still keep the actual link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<div
className={ clsx( 'dataviews-view-table__cell-content', {
'dataviews-view-table__cell-content--clickable':
isItemClickableField,
} ) }
tabIndex={ isItemClickableField ? 0 : undefined }
role="button"
onClick={ () => {
if ( isItemClickableField ) {
onItemClick( item );
}
} }
onKeyDown={ ( event ) => {
if (
( event.key === 'Enter' || event.key === '' ) &&
isItemClickableField
) {
onItemClick( item );
}
} }
>
<field.render { ...{ item } } />
</div>

We could wrap the field render in a Link component, but I'm not sure. The primary field might contain another a element, which could trigger a ValidateDomNesting warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's not ideal!
I am still leaning towards the fact that this should continue to be handled on a field level and maybe we only expose the onItemClick function that can be used as a callback with the field being clicked.

In order for us to do this, we did have to pass down a onClick function to the field render method.
Alluding to @oandregal last thought, maybe we want to pass down a onEvent callback that takes the event type click or something else, and the field.
The render method within the field can decide how to handle this.

) : (
<span>
{ decodeEntities( renderedTitle ) ||
__( '(no title)' ) }
</span>
);

let suffix = '';
if ( item.id === frontPageId ) {
Expand All @@ -210,7 +185,10 @@ function usePostFields( viewType ) {
alignment="center"
justify="flex-start"
>
{ title }
<span>
{ decodeEntities( renderedTitle ) ||
__( '(no title)' ) }
</span>
{ suffix }
</HStack>
);
Expand Down Expand Up @@ -355,7 +333,7 @@ function usePostFields( viewType ) {
},
passwordField,
],
[ authors, viewType, frontPageId, postsPageId ]
[ authors, frontPageId, postsPageId ]
);

return {
Expand Down
Loading
Loading