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

Add: Media field changing ui to Dataviews and content preview field to posts and pages #67278

Merged
merged 10 commits into from
Jan 7, 2025
Merged

Add: Media field changing ui to Dataviews and content preview field to posts and pages #67278

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cfe2cc0
Add: Media field changing ui to Dataviews and content preview field t…
jorgefilipecosta Dec 16, 2024
e5f07a3
update menu code
jorgefilipecosta Dec 17, 2024
5de23a1
naming changes
jorgefilipecosta Dec 17, 2024
a604caf
comment specifying is intereacting
jorgefilipecosta Dec 17, 2024
fd31448
feedbacl
jorgefilipecosta Dec 17, 2024
38e8ece
add media type remove flag
jorgefilipecosta Dec 27, 2024
757d1fc
refect field item logic
jorgefilipecosta Dec 27, 2024
24bbb1d
Document BasePost interface
jorgefilipecosta Dec 27, 2024
722ac27
fix focus loose and unable to close panel
jorgefilipecosta Jan 7, 2025
f713ffe
lint fix
jorgefilipecosta Jan 7, 2025
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
197 changes: 161 additions & 36 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
@@ -1,7 +1,8 @@
/**
* External dependencies
*/
import type { ChangeEvent } from 'react';
import type { ChangeEvent, ReactNode } from 'react';
import clsx from 'clsx';

/**
* WordPress dependencies
Expand All @@ -26,14 +27,15 @@ import {
Icon,
} from '@wordpress/components';
import { __, _x, sprintf } from '@wordpress/i18n';
import { memo, useContext, useMemo } from '@wordpress/element';
import { memo, useContext, useMemo, useState } from '@wordpress/element';
import {
chevronDown,
chevronUp,
cog,
seen,
unseen,
lock,
moreVertical,
} from '@wordpress/icons';
import warning from '@wordpress/warning';
import { useInstanceId } from '@wordpress/compose';
Expand Down Expand Up @@ -253,25 +255,77 @@ function ItemsPerPageControl() {
);
}

function PreviewOptions( {
previewOptions,
onChangePreviewOption,
onMenuOpenChange,
activeOption,
}: {
previewOptions?: Array< { label: string; id: string } >;
onChangePreviewOption?: ( newPreviewOption: string ) => void;
onMenuOpenChange: ( isOpen: boolean ) => void;
activeOption?: string;
} ) {
return (
<Menu onOpenChange={ onMenuOpenChange }>
<Menu.TriggerButton
render={
<Button
size="compact"
icon={ moreVertical }
label={ __( 'Preview' ) }
/>
}
/>
<Menu.Popover>
{ previewOptions?.map( ( { id, label } ) => {
return (
<Menu.RadioItem
key={ id }
value={ id }
checked={ id === activeOption }
onChange={ () => {
onChangePreviewOption?.( id );
} }
>
<Menu.ItemLabel>{ label }</Menu.ItemLabel>
</Menu.RadioItem>
);
} ) }
</Menu.Popover>
</Menu>
);
}
function FieldItem( {
field,
label,
description,
isVisible,
isFirst,
isLast,
canMove = true,
onToggleVisibility,
onMoveUp,
onMoveDown,
previewOptions,
onChangePreviewOption,
}: {
field: NormalizedField< any >;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this and add more individual props? If anything we should aim for the opposite if possible IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @ntsekouras, because the "Preview" item we create with a label sublabel/description etc is not a field, this component receives strings that can come from real field properties.

label?: string;
description?: string;
isVisible: boolean;
isFirst?: boolean;
isLast?: boolean;
canMove?: boolean;
onToggleVisibility?: () => void;
onMoveUp?: () => void;
onMoveDown?: () => void;
previewOptions?: Array< { label: string; id: string } >;
onChangePreviewOption?: ( newPreviewOption: string ) => void;
} ) {
const [ isChangingPreviewOption, setIsChangingPreviewOption ] =
useState< boolean >( false );

const focusVisibilityField = () => {
// 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.
Expand All @@ -290,16 +344,33 @@ function FieldItem( {
<Item>
<HStack
expanded
className={ `dataviews-field-control__field dataviews-field-control__field-${ field.id }` }
className={ clsx(
'dataviews-field-control__field',
`dataviews-field-control__field-${ field.id }`,
// The actions are hidden when the mouse is not hovering the item, or focus
// is outside the item.
// For actions that require a popover, a menu etc, that would mean that when the interactive element
// opens and the focus goes there the actions would be hidden.
// To avoid that we add a class to the item, that makes sure actions are visible while there is some
// interaction with the item.
{ 'is-interacting': isChangingPreviewOption }
) }
justify="flex-start"
>
<span className="dataviews-field-control__icon">
{ ! canMove && ! field.enableHiding && (
<Icon icon={ lock } />
) }
</span>
<span className="dataviews-field-control__label">
{ field.label }
<span className="dataviews-field-control__label-sub-label-container">
<span className="dataviews-field-control__label">
{ label || field.label }
</span>
{ description && (
<span className="dataviews-field-control__sub-label">
{ description }
</span>
) }
</span>
<HStack
justify="flex-end"
Expand Down Expand Up @@ -368,6 +439,14 @@ function FieldItem( {
}
/>
) }
{ previewOptions && (
<PreviewOptions
previewOptions={ previewOptions }
onChangePreviewOption={ onChangePreviewOption }
onMenuOpenChange={ setIsChangingPreviewOption }
activeOption={ field.id }
/>
) }
</HStack>
</HStack>
</Item>
Expand Down Expand Up @@ -461,7 +540,8 @@ function FieldControl() {
const hiddenFields = fields.filter(
( f ) =>
! visibleFieldIds.includes( f.id ) &&
! togglableFields.includes( f.id )
! togglableFields.includes( f.id ) &&
f.type !== 'media'
);
const visibleFields = visibleFieldIds
.map( ( fieldId ) => fields.find( ( f ) => f.id === fieldId ) )
Expand All @@ -471,18 +551,50 @@ function FieldControl() {
return null;
}
const titleField = fields.find( ( f ) => f.id === view.titleField );
const mediaField = fields.find( ( f ) => f.id === view.mediaField );
const previewField = fields.find( ( f ) => f.id === view.mediaField );
const descriptionField = fields.find(
( f ) => f.id === view.descriptionField
);

const previewFields = fields.filter( ( f ) => f.type === 'media' );

let previewFieldUI;
if ( previewFields.length > 1 ) {
const isPreviewFieldVisible =
isDefined( previewField ) && ( view.showMedia ?? true );
previewFieldUI = isDefined( previewField ) && (
<FieldItem
key={ previewField.id }
field={ previewField }
label={ __( 'Preview' ) }
description={ previewField.label }
isVisible={ isPreviewFieldVisible }
onToggleVisibility={ () => {
onChangeView( {
...view,
showMedia: ! isPreviewFieldVisible,
} );
} }
canMove={ false }
previewOptions={ previewFields.map( ( field ) => ( {
label: field.label,
id: field.id,
} ) ) }
onChangePreviewOption={ ( newPreviewId ) =>
onChangeView( { ...view, mediaField: newPreviewId } )
}
/>
);
}
const lockedFields = [
{
field: titleField,
isVisibleFlag: 'showTitle',
},
{
field: mediaField,
field: previewField,
isVisibleFlag: 'showMedia',
ui: previewFieldUI,
Copy link
Member

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 simplify this implementation? For example, instead of adding a specific UI for a field, what if we made the following changes:

FieldItem:

  • add a new property "description" that renders the field in use (e.g.: Featured image).
  • add a new property "options" that receives an array with the available (e.g.: [ "Content preview", "Featured image" ])
  • add a new property "onChangeOption" that communicates a new option has been selected

In the FieldControl, we do the following for the media field:

<FieldItem
  field={ field }
  description={ view.mediaField.label }
  options=[
    { label:"Content preview", id: content_preview },
    { label: "Featured image", id: feature_image }
  ],
  onChangeOption={ newOption => onChangeView({ mediaField: newOption })
/>

I feel the implementation would be simpler if we make the FieldItem (leaf node) a general component that the FieldControl configures depending on the specifics of the field (visible, not visible, can have multiple options, etc.). This way, it's easy to add the same behaviour to other fields (title).

Copy link
Member Author

Choose a reason for hiding this comment

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

I applied this suggestion, It had some complexities but the base idea is there.

},
{
field: descriptionField,
Expand All @@ -493,12 +605,20 @@ function FieldControl() {
( { field, isVisibleFlag } ) =>
// @ts-expect-error
isDefined( field ) && ( view[ isVisibleFlag ] ?? true )
) as Array< { field: NormalizedField< any >; isVisibleFlag: string } >;
) as Array< {
field: NormalizedField< any >;
isVisibleFlag: string;
ui?: ReactNode;
} >;
const hiddenLockedFields = lockedFields.filter(
( { field, isVisibleFlag } ) =>
// @ts-expect-error
isDefined( field ) && ! ( view[ isVisibleFlag ] ?? true )
) as Array< { field: NormalizedField< any >; isVisibleFlag: string } >;
) as Array< {
field: NormalizedField< any >;
isVisibleFlag: string;
ui?: ReactNode;
} >;

return (
<VStack className="dataviews-field-control" spacing={ 6 }>
Expand All @@ -507,20 +627,22 @@ function FieldControl() {
!! visibleFields?.length ) && (
<ItemGroup isBordered isSeparated>
{ visibleLockedFields.map(
( { field, isVisibleFlag } ) => {
( { field, isVisibleFlag, ui } ) => {
return (
<FieldItem
key={ field.id }
field={ field }
isVisible
onToggleVisibility={ () => {
onChangeView( {
...view,
[ isVisibleFlag ]: false,
} );
} }
canMove={ false }
/>
ui ?? (
<FieldItem
key={ field.id }
field={ field }
isVisible
onToggleVisibility={ () => {
onChangeView( {
...view,
[ isVisibleFlag ]: false,
} );
} }
canMove={ false }
/>
)
);
}
) }
Expand Down Expand Up @@ -550,20 +672,23 @@ function FieldControl() {
<ItemGroup isBordered isSeparated>
{ hiddenLockedFields.length > 0 &&
hiddenLockedFields.map(
( { field, isVisibleFlag } ) => {
( { field, isVisibleFlag, ui } ) => {
return (
<FieldItem
key={ field.id }
field={ field }
isVisible={ false }
onToggleVisibility={ () => {
onChangeView( {
...view,
[ isVisibleFlag ]: true,
} );
} }
canMove={ false }
/>
ui ?? (
<FieldItem
key={ field.id }
field={ field }
isVisible={ false }
onToggleVisibility={ () => {
onChangeView( {
...view,
[ isVisibleFlag ]:
true,
} );
} }
canMove={ false }
/>
)
);
}
) }
Expand Down
17 changes: 15 additions & 2 deletions packages/dataviews/src/components/dataviews-view-config/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@
}

.dataviews-field-control__field:hover,
.dataviews-field-control__field:focus-within {
.dataviews-field-control__field:focus-within,
.dataviews-field-control__field.is-interacting {
.dataviews-field-control__actions {
position: unset;
top: unset;
Expand All @@ -80,6 +81,18 @@
width: $icon-size;
}

.dataviews-field-control__label {
.dataviews-field-control__label-sub-label-container {
flex-grow: 1;
}

.dataviews-field-control__label {
display: block;
}

.dataviews-field-control__sub-label {
margin-top: $grid-unit-10;
margin-bottom: 0;
font-size: 11px;
font-style: normal;
color: $gray-700;
}
2 changes: 1 addition & 1 deletion packages/dataviews/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export type Operator =
| 'isAll'
| 'isNotAll';

export type FieldType = 'text' | 'integer' | 'datetime';
Copy link
Member

@oandregal oandregal Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, this comes with its own field-type definition. However, we don't need that for this PR. To control scope, we can do it in a follow-up. There're other changes we may want to do for a media type definition, like making it not sortable by default, etc.

export type FieldType = 'text' | 'integer' | 'datetime' | 'media';

export type ValidationContext = {
elements?: Option[];
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ _Parameters_
- _props.post_ `[Object]`: The post object to edit. This is required.
- _props.\_\_unstableTemplate_ `[Object]`: The template object wrapper the edited post. This is optional and can only be used when the post type supports templates (like posts and pages).
- _props.settings_ `[Object]`: The settings object to use for the editor. This is optional and can be used to override the default settings.
- _props.children_ `[Element]`: Children elements for which the BlockEditorProvider context should apply. This is optional.
- _props.children_ `[React.ReactNode]`: Children elements for which the BlockEditorProvider context should apply. This is optional.

_Returns_

Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ export const ExperimentalEditorProvider = withRegistryProvider(
* This is optional and can only be used when the post type supports templates (like posts and pages).
* @param {Object} [props.settings] The settings object to use for the editor.
* This is optional and can be used to override the default settings.
* @param {Element} [props.children] Children elements for which the BlockEditorProvider context should apply.
* @param {React.ReactNode} [props.children] Children elements for which the BlockEditorProvider context should apply.
* This is optional.
*
* @example
Expand Down
Loading
Loading