Skip to content

DataViews: Type the ViewActions component #61729

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

Merged
youknowriad merged 2 commits into from
May 20, 2024
Merged

DataViews: Type the ViewActions component #61729

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
fc1138c
DataViews: Type the ViewActions component
youknowriad May 16, 2024
b12af1c
Apply review suggestion
youknowriad May 20, 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
0 packages/dataviews/src/layouts.js → packages/dataviews/src/layouts.ts
View file
Open in desktop
File renamed without changes.
16 changes: 8 additions & 8 deletions packages/dataviews/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,12 @@ export interface ViewTable extends ViewBase {
/**
* The field to use as the primary field.
*/
primaryField: string;
primaryField?: string;
Copy link
Contributor Author

@youknowriad youknowriad May 16, 2024

Choose a reason for hiding this comment

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

When switching view "type", I had to reset the "layout" object because that object is supposed to be different from one view type to another. This forced me to mark all the layout subkeys as optional.

I'm guessing the other options is to have some kind of "defaultLayout" provided per view type but I believe this approach is simpler and does the job for now.


/**
* The field to use as the media field.
*/
mediaField: string;
mediaField?: string;
};
}

Expand All @@ -206,12 +206,12 @@ export interface ViewList extends ViewBase {
/**
* The field to use as the primary field.
*/
primaryField: string;
primaryField?: string;

/**
* The field to use as the media field.
*/
mediaField: string;
mediaField?: string;
};
}

Expand All @@ -222,22 +222,22 @@ export interface ViewGrid extends ViewBase {
/**
* The field to use as the primary field.
*/
primaryField: string;
primaryField?: string;

/**
* The field to use as the media field.
*/
mediaField: string;
mediaField?: string;

/**
* The fields to use as columns.
*/
columnFields: string[];
columnFields?: string[];

/**
* The fields to use as badge fields.
*/
badgeFields: string[];
badgeFields?: string[];
};
}

Expand Down
79 changes: 67 additions & 12 deletions packages/dataviews/src/view-actions.js → packages/dataviews/src/view-actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import type { ChangeEvent } from 'react';

/**
* WordPress dependencies
*/
Expand All @@ -15,6 +20,7 @@ import { settings } from '@wordpress/icons';
import { unlock } from './lock-unlock';
import { SORTING_DIRECTIONS, sortLabels } from './constants';
import { VIEW_LAYOUTS } from './layouts';
import type { AnyItem, NormalizedField, View } from './types';

const {
DropdownMenuV2: DropdownMenu,
Expand All @@ -25,7 +31,41 @@ const {
DropdownMenuItemLabelV2: DropdownMenuItemLabel,
} = unlock( componentsPrivateApis );

function ViewTypeMenu( { view, onChangeView, supportedLayouts } ) {
interface ViewTypeMenuProps {
view: View;
onChangeView: ( view: View ) => void;
supportedLayouts?: string[];
}

interface PageSizeMenuProps {
view: View;
onChangeView: ( view: View ) => void;
}

interface FieldsVisibilityMenuProps< Item extends AnyItem > {
view: View;
onChangeView: ( view: View ) => void;
fields: NormalizedField< Item >[];
}

interface SortMenuProps< Item extends AnyItem > {
fields: NormalizedField< Item >[];
view: View;
onChangeView: ( view: View ) => void;
}

interface ViewActionsProps< Item extends AnyItem > {
fields: NormalizedField< Item >[];
view: View;
onChangeView: ( view: View ) => void;
supportedLayouts?: string[];
}

function ViewTypeMenu( {
view,
onChangeView,
supportedLayouts,
}: ViewTypeMenuProps ) {
let _availableViews = VIEW_LAYOUTS;
if ( supportedLayouts ) {
_availableViews = _availableViews.filter( ( _view ) =>
Expand All @@ -41,7 +81,7 @@ function ViewTypeMenu( { view, onChangeView, supportedLayouts } ) {
trigger={
<DropdownMenuItem
suffix={
<span aria-hidden="true">{ activeView.label }</span>
<span aria-hidden="true">{ activeView?.label }</span>
}
>
<DropdownMenuItemLabel>
Expand All @@ -58,11 +98,18 @@ function ViewTypeMenu( { view, onChangeView, supportedLayouts } ) {
name="view-actions-available-view"
checked={ availableView.type === view.type }
hideOnClick
onChange={ ( e ) => {
onChangeView( {
...view,
type: e.target.value,
} );
onChange={ ( e: ChangeEvent< HTMLInputElement > ) => {
Copy link
Contributor Author

@youknowriad youknowriad May 16, 2024

Choose a reason for hiding this comment

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

The main challenge of this PR is this function: Basically how to keep the "view" object valid (correct type) when switching view types.

Copy link
Member

@sirreal sirreal May 20, 2024

Choose a reason for hiding this comment

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

Transitional states are difficult, this seems to highlight complexity that wasn't apparent, although it seems like the complexity isn't related to types as much as it is to application states. What should that layout look like?

If the layout properties are truly optional then this makes sense and may be better than the previous behavior where dataviews would maintain layout properties when switching views.

I know this isn't your question, but I try to avoid type assertions like these and would typically do something like this here:

onChange={ ( e: ChangeEvent< HTMLInputElement > ) => {
	switch ( e.target.value ) {
		case 'list':
		case 'grid':
		case 'table':
			return onChangeView( {
				...view,
				type: e.target.value,
				layout: {},
			} );
	}
	throw new Error( 'Invalid dataview' );
} }

Copy link
Contributor Author

@youknowriad youknowriad May 20, 2024

Choose a reason for hiding this comment

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

I agree the complexity is on the app level: "how to switch view types when the view types can be different" and we don't have a good answer yet.

switch ( e.target.value ) {
case 'list':
case 'grid':
case 'table':
return onChangeView( {
...view,
type: e.target.value,
layout: {},
} );
}
throw new Error( 'Invalid dataview' );
} }
>
<DropdownMenuItemLabel>
Expand All @@ -76,7 +123,7 @@ function ViewTypeMenu( { view, onChangeView, supportedLayouts } ) {
}

const PAGE_SIZE_VALUES = [ 10, 20, 50, 100 ];
function PageSizeMenu( { view, onChangeView } ) {
function PageSizeMenu( { view, onChangeView }: PageSizeMenuProps ) {
return (
<DropdownMenu
trigger={
Expand Down Expand Up @@ -114,7 +161,11 @@ function PageSizeMenu( { view, onChangeView } ) {
);
}

function FieldsVisibilityMenu( { view, onChangeView, fields } ) {
function FieldsVisibilityMenu< Item extends AnyItem >( {
view,
onChangeView,
fields,
}: FieldsVisibilityMenuProps< Item > ) {
const hidableFields = fields.filter(
( field ) =>
field.enableHiding !== false && field.id !== view.layout.mediaField
Expand Down Expand Up @@ -164,7 +215,11 @@ function FieldsVisibilityMenu( { view, onChangeView, fields } ) {
);
}

function SortMenu( { fields, view, onChangeView } ) {
function SortMenu< Item extends AnyItem >( {
fields,
view,
onChangeView,
}: SortMenuProps< Item > ) {
const sortableFields = fields.filter(
( field ) => field.enableSorting !== false
);
Expand Down Expand Up @@ -248,12 +303,12 @@ function SortMenu( { fields, view, onChangeView } ) {
);
}

const ViewActions = memo( function ViewActions( {
const ViewActions = memo( function ViewActions< Item extends AnyItem >( {
fields,
view,
onChangeView,
supportedLayouts,
} ) {
}: ViewActionsProps< Item > ) {
return (
<DropdownMenu
trigger={
Expand Down
2 changes: 1 addition & 1 deletion packages/dataviews/src/view-grid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ interface GridItemProps< Item extends AnyItem > {
primaryField?: NormalizedField< Item >;
visibleFields: NormalizedField< Item >[];
badgeFields: NormalizedField< Item >[];
columnFields: string[];
columnFields?: string[];
}

function GridItem< Item extends AnyItem >( {
Expand Down