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 field reordering and visibility logic #64999

Merged
merged 12 commits into from
Sep 3, 2024
Merged

DataViews: fix field reordering and visibility logic #64999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
efbac20
Centralize logic in FieldControl
oandregal Sep 2, 2024
6042f92
Logic for combined fields
oandregal Sep 2, 2024
e1d2cd7
Add comment
oandregal Sep 2, 2024
7a47ded
Extract utils
oandregal Sep 3, 2024
56d8a67
Table: use getVisibleFieldIds util
oandregal Sep 3, 2024
27a1b8d
Sort properties by view.fields order
oandregal Sep 3, 2024
d8549d9
Hidden fields should not include the special media field
oandregal Sep 3, 2024
98ec9d3
Allow re-ordering when view.fields is undefined
oandregal Sep 3, 2024
4a0602d
Consider combined field in visibleFields if no view.fields is provided
oandregal Sep 3, 2024
ed9b897
Use fieldId instead of field.id
oandregal Sep 3, 2024
d34fec7
Fix column reordering when no view.fields is provided
oandregal Sep 3, 2024
2c2d801
Display moving controls for combined fields
oandregal Sep 3, 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
251 changes: 113 additions & 138 deletions packages/dataviews/src/components/dataviews-view-config/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,13 @@ import {
sortIcons,
sortLabels,
} from '../../constants';
import { VIEW_LAYOUTS, getMandatoryFields } from '../../dataviews-layouts';
import type { NormalizedField, SupportedLayouts, View } from '../../types';
import {
VIEW_LAYOUTS,
getNotHidableFieldIds,
getVisibleFieldIds,
getHiddenFieldIds,
} from '../../dataviews-layouts';
import type { SupportedLayouts, View, Field } from '../../types';
import DataViewsContext from '../dataviews-context';
import { unlock } from '../../lock-unlock';
import DensityPicker from '../../dataviews-layouts/grid/density-picker';
Expand Down Expand Up @@ -232,50 +237,34 @@ function ItemsPerPageControl() {
);
}

interface FieldItemProps {
id: any;
label: string;
index: number;
isVisible: boolean;
isHidable: boolean;
}

function FieldItem( {
field: { id, label, index, isVisible, isHidable },
fields,
fieldId,
mandatoryFields,
viewFields,
view,
onChangeView,
}: {
fields: NormalizedField< any >[];
fieldId: string;
mandatoryFields: string | any[];
viewFields: string[];
field: FieldItemProps;
fields: Field< any >[];
view: View;
onChangeView: ( view: View ) => void;
} ) {
let fieldLabel;
let fieldIsHidable;
const fieldObject = fields.find(
( f ) => f.id === fieldId
) as NormalizedField< any >;
if ( fieldObject ) {
fieldLabel = fieldObject.label;
fieldIsHidable =
fieldObject.enableHiding !== false &&
! mandatoryFields.includes( fieldId );
} else if ( view.type === LAYOUT_TABLE ) {
const combinedFieldObject = view.layout?.combinedFields?.find(
( f ) => f.id === fieldId
);
if ( combinedFieldObject ) {
fieldLabel = combinedFieldObject.label;
fieldIsHidable = ! mandatoryFields.includes( fieldId );
}
}
const visibleFieldIds = getVisibleFieldIds( view, fields );

const index = view.fields?.indexOf( fieldId ) as number;
const isVisible = viewFields.includes( fieldId );
return (
<Item key={ fieldId }>
<Item key={ id }>
<HStack
expanded
className={ `dataviews-field-control__field dataviews-field-control__field-${ fieldId }` }
className={ `dataviews-field-control__field dataviews-field-control__field-${ id }` }
>
<span>{ fieldLabel }</span>
<span>{ label }</span>
<HStack
justify="flex-end"
expanded={ false }
Expand All @@ -284,90 +273,81 @@ function FieldItem( {
{ view.type === LAYOUT_TABLE && isVisible && (
<>
<Button
disabled={ ! isVisible || index < 1 }
disabled={ index < 1 }
accessibleWhenDisabled
size="compact"
onClick={ () => {
if ( ! view.fields || index < 1 ) {
return;
}
onChangeView( {
...view,
fields: [
...( view.fields.slice(
...( visibleFieldIds.slice(
0,
index - 1
) ?? [] ),
fieldId,
view.fields[ index - 1 ],
...view.fields.slice( index + 1 ),
id,
visibleFieldIds[ index - 1 ],
...visibleFieldIds.slice(
index + 1
),
],
} );
} }
icon={ chevronUp }
label={ sprintf(
/* translators: %s: field label */
__( 'Move %s up' ),
fieldLabel
label
) }
/>
<Button
disabled={
! isVisible ||
! view.fields ||
index >= view.fields.length - 1
}
disabled={ index >= visibleFieldIds.length - 1 }
accessibleWhenDisabled
size="compact"
onClick={ () => {
if (
! view.fields ||
index >= view.fields.length - 1
) {
return;
}
onChangeView( {
...view,
fields: [
...( view.fields.slice(
...( visibleFieldIds.slice(
0,
index
) ?? [] ),
view.fields[ index + 1 ],
fieldId,
...view.fields.slice( index + 2 ),
visibleFieldIds[ index + 1 ],
id,
...visibleFieldIds.slice(
index + 2
),
],
} );
} }
icon={ chevronDown }
label={ sprintf(
/* translators: %s: field label */
__( 'Move %s down' ),
fieldLabel
label
) }
/>{ ' ' }
</>
) }
<Button
className="dataviews-field-control__field-visibility-button"
disabled={ ! fieldIsHidable }
disabled={ ! isHidable }
accessibleWhenDisabled
size="compact"
onClick={ () => {
onChangeView( {
...view,
fields: isVisible
? viewFields.filter(
( id ) => id !== fieldId
? visibleFieldIds.filter(
( fieldId ) => fieldId !== id
)
: [ ...viewFields, fieldId ],
: [ ...visibleFieldIds, id ],
} );
// Focus the visibility button to avoid focus loss.
// Our code is safe against the component being unmounted, so we don't need to worry about cleaning the timeout.
// eslint-disable-next-line @wordpress/react-no-unsafe-timeout
setTimeout( () => {
const element = document.querySelector(
`.dataviews-field-control__field-${ fieldId } .dataviews-field-control__field-visibility-button`
`.dataviews-field-control__field-${ id } .dataviews-field-control__field-visibility-button`
);
if ( element instanceof HTMLElement ) {
element.focus();
Expand All @@ -380,12 +360,12 @@ function FieldItem( {
? sprintf(
/* translators: %s: field label */
__( 'Hide %s' ),
fieldLabel
label
)
: sprintf(
/* translators: %s: field label */
__( 'Show %s' ),
fieldLabel
label
)
}
/>
Expand All @@ -395,83 +375,77 @@ function FieldItem( {
);
}

function FieldList( {
fields,
fieldIds,
mandatoryFields,
viewFields,
view,
onChangeView,
}: Omit< Parameters< typeof FieldItem >[ 0 ], 'fieldId' > & {
fieldIds: string[];
} ) {
return fieldIds.map( ( fieldId ) => (
<FieldItem
key={ fieldId }
fields={ fields }
fieldId={ fieldId }
mandatoryFields={ mandatoryFields }
viewFields={ viewFields }
view={ view }
onChangeView={ onChangeView }
/>
) );
}

function FieldControl() {
const { view, fields, onChangeView } = useContext( DataViewsContext );
const mandatoryFields = useMemo(
() => getMandatoryFields( view ),

const visibleFieldIds = useMemo(
() => getVisibleFieldIds( view, fields ),
[ view, fields ]
);
const hiddenFieldIds = useMemo(
() => getHiddenFieldIds( view, fields ),
[ view, fields ]
);
const notHidableFieldIds = useMemo(
() => getNotHidableFieldIds( view ),
[ view ]
);
const viewFields = view.fields || fields.map( ( field ) => field.id );
const visibleFields = view.fields;
const hiddenFields = useMemo( () => {
const nonViewFieldsList = fields
.filter(
( field ) =>
! viewFields.includes( field.id ) &&
! mandatoryFields?.includes( field.id )
)
.map( ( field ) => field.id );

if ( view.type !== LAYOUT_TABLE ) {
return nonViewFieldsList;
}
const nonViewFieldsAndNonCombinedList = nonViewFieldsList.filter(
( fieldId ) => {
return ! view.layout?.combinedFields?.some( ( combinedField ) =>
combinedField.children.includes( fieldId )
);
}
);
const nonViewFieldsCombinedFieldsList =
view.layout?.combinedFields
?.filter(
( combinedField ) =>
! viewFields.includes( combinedField.id )
)
.map( ( combinedField ) => combinedField.id ) || [];
return [
...nonViewFieldsAndNonCombinedList,
...nonViewFieldsCombinedFieldsList,
];
}, [ view, mandatoryFields, fields, viewFields ] );
const visibleFields = fields
.filter( ( { id } ) => visibleFieldIds.includes( id ) )
.map( ( { id, label, enableHiding } ) => {
return {
id,
label,
index: visibleFieldIds.indexOf( id ),
isVisible: true,
isHidable: notHidableFieldIds.includes( id )
? false
: enableHiding,
};
} );
if ( view.type === LAYOUT_TABLE && view.layout?.combinedFields ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this works for now, but my thinking is that ultimately, we should find a way for the layouts to actually "inject" logic here. Or (which is what I had in mind) to have some kind of a slot and the "properties" area be specific per layout. For instance in "table", we should be able to "combine" fields if we want.

Copy link
Member Author

@oandregal oandregal Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. If we aim to make the layouts "registerable/unregisterable" by 3rd parties we cannot have this kind of logic in the view config (or any other component).

I've looked at the current usage of layouts in this component and this is what I see:

All of them could be solved by providing a layout.supports thing somewhere. Using that approach, it'd still be DataViews' responsibility to define which "features" are supported.

Or (which is what I had in mind) to have some kind of a slot and the "properties" area be specific per layout. For instance in "table", we should be able to "combine" fields if we want.

This approach would be a lot more open. However, every layout would have to implement the same actions (reordering, visibility, etc.).

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, not for this PR 😅

Copy link
Member Author

@oandregal oandregal Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, layout.supports could also be a nice thing to have for consumers that want to "lock in" (aka remove options) a bit more the user experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach would be a lot more open. However, every layout would have to implement the same actions (reordering, visibility, etc.).

Yes, but for list view we have badges..., for grid we have columns and rows...

view.layout.combinedFields.forEach( ( { id, label } ) => {
visibleFields.push( {
id,
label,
index: visibleFieldIds.indexOf( id ),
isVisible: true,
isHidable: notHidableFieldIds.includes( id ),
} );
} );
}
visibleFields.sort( ( a, b ) => a.index - b.index );

const hiddenFields = fields
.filter( ( { id } ) => hiddenFieldIds.includes( id ) )
.map( ( { id, label, enableHiding }, index ) => {
return {
id,
label,
index,
isVisible: false,
isHidable: enableHiding,
};
} );

if ( ! visibleFields?.length && ! hiddenFields?.length ) {
return null;
}

return (
<VStack spacing={ 6 } className="dataviews-field-control">
{ !! visibleFields?.length && (
<ItemGroup isBordered isSeparated>
<FieldList
fields={ fields }
fieldIds={ visibleFields }
mandatoryFields={ mandatoryFields }
viewFields={ viewFields }
view={ view }
onChangeView={ onChangeView }
/>
{ visibleFields.map( ( field ) => (
<FieldItem
key={ field.id }
field={ field }
fields={ fields }
view={ view }
onChangeView={ onChangeView }
/>
) ) }
</ItemGroup>
) }
{ !! hiddenFields?.length && (
Expand All @@ -481,14 +455,15 @@ function FieldControl() {
{ __( 'Hidden' ) }
</BaseControl.VisualLabel>
<ItemGroup isBordered isSeparated>
<FieldList
fields={ fields }
fieldIds={ hiddenFields }
mandatoryFields={ mandatoryFields }
viewFields={ viewFields }
view={ view }
onChangeView={ onChangeView }
/>
{ hiddenFields.map( ( field ) => (
<FieldItem
key={ field.id }
field={ field }
fields={ fields }
view={ view }
onChangeView={ onChangeView }
/>
) ) }
</ItemGroup>
</VStack>
</>
Expand Down
Loading
Loading