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: add new field types and formats to modify the default render #56942

Closed
wants to merge 14 commits into from
4 changes: 4 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

71 changes: 58 additions & 13 deletions packages/dataviews/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,25 +150,16 @@ Example:
[
{
id: 'date',
type: 'date',
header: __( 'Date' ),
getValue: ( { item } ) => item.date,
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be optional too because it's just item[ id ]

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 it's good to keep them both because it's just a coincidence that we the id here has the same type. This kind of shortcut would work for just a handful of cases(only the field types we support).

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 it would work in most cases, I'm not talking about reusing the "type", but using the "id" instead.

render: ( { item } ) => {
return (
<time>{ getFormattedDate( item.date ) }</time>
);
},
enableHiding: false
},
{
id: 'author',
type: 'enumeration',
header: __( 'Author' ),
getValue: ( { item } ) => item.author,
render: ( { item } ) => {
return (
<a href="...">{ item.author }</a>
);
},
type: 'enumeration',
elements: [
{ value: 1, label: 'Admin' }
{ value: 2, label: 'User' }
Expand All @@ -179,11 +170,11 @@ Example:
```

- `id`: identifier for the field. Unique.
- `type`: the type of the field. Used to render the field and generate the proper filters. See "Field types".
- `header`: the field's name to be shown in the UI.
- `getValue`: function that returns the value of the field.
- `render`: function that renders the field.
- `formats`: a list of modifications to the default render provided for the field. See "Format".
- `elements`: the set of valid values for the field's value.
- `type`: the type of the field. Used to generate the proper filters. Only `enumeration` available at the moment. See "Field types".
- `enableSorting`: whether the data can be sorted by the given field. True by default.
- `enableHiding`: whether the field can be hidden. True by default.
- `filterBy`: configuration for the filters.
Expand Down Expand Up @@ -211,10 +202,64 @@ Array of operations that can be performed upon each record. Each action is an ob
- `list`: the view uses a list layout.
- Field types:
- `enumeration`: the field value should be taken and can be filtered from a closed list of elements.
- `date`: the field value is a date.
- `text`: the field value is a string.
- `img`: the field value is an image.
- Operator types:
- `in`: operator to be used in filters for fields of type `enumeration`.
- `notIn`: operator to be used in filters for fields of type `enumeration`.

## Formats
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to wrap my ahead around this abstraction. Why do you think we need something like this instead of just overriding the "render" function provider by the "type"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also love some more info about formats. In my mind if a field provides a render function should go first, then the field types render if exists and then the getValue.

Copy link
Member Author

Choose a reason for hiding this comment

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

The question I'm trying to answer with this experiment is: can we have a fields API without render?

I've shared the current issue we have in the Why section of the PR's description with more detail, the TLDR:

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of consumers overriding the render function to add a link, they would declare a link format. What's the value of this approach? That the markup produced by the fields is controlled by the views and not the fields.

Sharing some issues off the top of my head to illustrate the point:

  • control over keyboard navigation and interactivity: for example, how do we make sure the primary field of the list view is not clickable? If the link is provided in a structured data (a format), the list view can decide not to render it.
  • accessibility: consumers don't have to provide for empty cells in the table layout for accessibility reasons.
  • accessibility: proper use of labels for fields because it's the view who controls the markup.
  • how fields are able to render in views they don't know that exist yet? For example, a 3rd party creates a new layout like a calendar. How the field knows whether this view should have links, the sizes for the images/etc.?

Copy link
Contributor

Choose a reason for hiding this comment

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

consumers don't have to provide — for empty cells in the table layout for accessibility reasons.

This could be a different separate API and I agree this should be handled internally. Maybe if we do make the getValue return the raw data used by each item would be enough to check if the field will be empty. Maybe in some other cases though this might not be enough and we need to make more checks.. Related to this I think the empty field should be handled by the views in a consistent way and not let fields decide what to render.

A similar problem we already have is how to determine if we need to render a placeholder in grid view, since render might be a component that uses hooks and we can't determine if the component renders null or something(the return value would be a react element).

accessibility: proper use of labels for fields because it's the view who controls the markup.

What is the problem with labels? And how it's solved with formats?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for sharing, the list of issues is growing :)

What is the problem with labels? And how it's solved with formats?

See comment by Andrew. Fields other than the primary need to point to the primaryField. Using an approach as formats, the view generates the markup. By having control of the generated markup, it can absorb all these concerns.

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 formats is not really clear as an API yet. I recognize the limitations of "extending render" but I also feel the formats API are mixing stuff that are unrelated and probably is an API that is very tailored towards what we currently have as fields.

Let's take the "link" format for instance, it assumes that we take the basic "render" function and wrap it with a link. What If I want to add a link to the "text" but add an icon to the right or something like that. We can't introspect the "children" to do these kind of things.

I also think a low level API like overriding "render" is something we will need anyway, because there will always be very custom field rendering.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's take the "link" format for instance, it assumes that we take the basic "render" function and wrap it with a link. What If I want to add a link to the "text" but add an icon to the right or something like that. We can't introspect the "children" to do these kind of things.

This is very similar to what the title in the templates page does, and it's implemented in this PoC using the link and after formats.

There's also a prefix format to do that sort of thing (adding an icon to the author field in the template page).

Copy link
Member Author

@oandregal oandregal Dec 12, 2023

Choose a reason for hiding this comment

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

I also think a low level API like overriding "render" is something we will need anyway, because there will always be very custom field rendering.

I created this PR because I had the same belief and I wanted to prove me wrong. I was able to implement everything we have now using formats, so I'm less convinced now than I was.

Where I see this approach falling short is in formatting parts of the content. For example, adding a link such as this:

Some text with <a href="">link</a>.

For that, an approach like this would not work. It can only work with whole nodes, so to speak: add new nodes in different ways (inline: prefix/suffix, block: before/after), extend the existing markup with props, etc.


Each format is an object whose shape is:

```js
{
type: 'format-type',
renderProps: ( { item } ) => { return { newProp: '...' }; },
renderChildren: ( { item } ) => { return Component; }
}
```

Existing formats are:

- `link`: format that declares the field should contain a link with the given props (href, etc.).

```js
{
type: 'link',
renderProps: ( { item } ) => ({ href: 'new-url-based-on-item' }),
},
```

- `empty`: format that declares the field wants to render an empty state different from the default.

```js
{
type: 'empty',
renderChildren: ( { item } ) => { return __( '(no title)' ); },
},
```

- `prefix`: format that renders a component as a prefix to the default render. Example: used to render an icon before.

```js
{
type: 'prefix',
renderChildren: ( { item } ) => { return <Icon icon={ iconBasedOnItem } >; },
}
```

- `after`: format that renders a component. Similar to CSS pseudo-element `::after`.

```js
{
type: 'after',
renderProps: ( { item } ) => ({ className: '...' }),
renderChildren: ( { item }) => { return <Label item={item}>; },
}
```

## Contributing to this package

This is an individual package that's part of the Gutenberg project. The project is organized as a monorepo. It's made up of multiple self-contained software packages, each with a specific purpose. The packages in this monorepo are published to [npm](https://www.npmjs.com/) and used by [WordPress](https://make.wordpress.org/core/) as well as other software projects.
Expand Down
2 changes: 2 additions & 0 deletions packages/dataviews/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@
"@wordpress/a11y": "file:../a11y",
"@wordpress/components": "file:../components",
"@wordpress/compose": "file:../compose",
"@wordpress/date": "file:../date",
"@wordpress/element": "file:../element",
"@wordpress/html-entities": "file:../html-entities",
"@wordpress/i18n": "file:../i18n",
"@wordpress/icons": "file:../icons",
"@wordpress/keycodes": "file:../keycodes",
Expand Down
2 changes: 2 additions & 0 deletions packages/dataviews/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import ViewList from './view-list';

// Field types.
export const ENUMERATION_TYPE = 'enumeration';
export const DATE_TYPE = 'date';
export const TEXT_TYPE = 'text';

// Filter operators.
export const OPERATOR_IN = 'in';
Expand Down
41 changes: 34 additions & 7 deletions packages/dataviews/src/dataviews.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ import Pagination from './pagination';
import ViewActions from './view-actions';
import Filters from './filters';
import Search from './search';
import { VIEW_LAYOUTS } from './constants';
import {
VIEW_LAYOUTS,
DATE_TYPE,
ENUMERATION_TYPE,
TEXT_TYPE,
} from './constants';
import { renderDate, renderEnumeration, renderText } from './types';

export default function DataViews( {
view,
Expand Down Expand Up @@ -42,11 +48,32 @@ export default function DataViews( {
( v ) => v.type === view.type
).component;
const _fields = useMemo( () => {
return fields.map( ( field ) => ( {
...field,
render: field.render || field.getValue,
} ) );
return fields.map( ( field ) => {
// Normalize formats.
field.formats = field.formats || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a mutation during render, probably something that can cause bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

you should do something like const result = { ...field } and mutate result instead.


// Normalize render.
switch ( field.type ) {
case DATE_TYPE:
field.render = ( { item } ) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why we're updating the "field" here instead of doing something like:

const fieldType = getFieldType( field.type };
fieldType.render();

It seems to me that it's better to keep things clear as otherwise, we'll be wondering is this "field" the full field or the one with optional stuff...

renderDate( { field, item } );
break;
case ENUMERATION_TYPE:
field.render = ( { item } ) =>
renderEnumeration( { field, item } );
break;
case TEXT_TYPE:
field.render = ( { item } ) =>
renderText( { field, item } );
break;
default:
field.render = field.render || field.getValue;
}

return field;
} );
}, [ fields ] );

return (
<div className="dataviews-wrapper">
<VStack spacing={ 0 } justify="flex-start">
Expand All @@ -63,13 +90,13 @@ export default function DataViews( {
/>
) }
<Filters
fields={ fields }
fields={ _fields }
view={ view }
onChangeView={ onChangeView }
/>
</HStack>
<ViewActions
fields={ fields }
fields={ _fields }
view={ view }
onChangeView={ onChangeView }
supportedLayouts={ supportedLayouts }
Expand Down
114 changes: 114 additions & 0 deletions packages/dataviews/src/types.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/**
* WordPress dependencies
*/
import { dateI18n, getDate, getSettings } from '@wordpress/date';
import {
__experimentalVStack as VStack,
__experimentalHStack as HStack,
__experimentalText as Text,
VisuallyHidden,
} from '@wordpress/components';
import { decodeEntities } from '@wordpress/html-entities';
import { sprintf, __ } from '@wordpress/i18n';

const renderLinkFormat = ( { format, item, children } ) => {
const props = format.renderProps( { item } );
return <a { ...props }>{ children }</a>;
};

const renderDateFormat = ( { children } ) => {
const formattedDate = dateI18n(
getSettings().formats.datetimeAbbreviated,
getDate( children )
);
return <time>{ formattedDate }</time>;
};

const renderAfterFormat = ( { format, item } ) => {
const props = format.renderProps( { item } );
return <span { ...props }>{ format.renderChildren( { item } ) }</span>;
};

// TODO: remove field prop?
const renderEmptyFormat = ( { format, item, field } ) => {
if ( format ) {
const props = format.renderProps ? format.renderProps( { item } ) : {};
return <Text { ...props }>{ format.renderChildren( { item } ) }</Text>;
}

return (
<>
<Text variant="muted" aria-hidden="true">
&#8212;
</Text>
<VisuallyHidden>
{ sprintf(
/* translators: %s: field description or field id, e.g.: "No author." */
__( 'No %s.' ),
field.description?.toLowerCase() || field.id
) }
</VisuallyHidden>
</>
);
};

const renderPrefixFormat = ( { format, item, children } ) => {
return (
<HStack alignment="left" spacing={ 1 }>
{ format.renderChildren( { item } ) }
<span>{ children }</span>
</HStack>
);
};

export const renderDate = ( { field, item } ) => {
field.formats.unshift( { type: 'date' } );

return renderText( { field, item } );
};

export const renderText = ( { field, item } ) => {
const value = decodeEntities( field.getValue( { item } ) );
if ( ! value ) {
const emptyFormat = field.formats.find(
( format ) => format.type === 'empty'
);
return renderEmptyFormat( { format: emptyFormat, item, field } );
}

const result = field.formats.reduce( ( acc, format ) => {
if ( format.type === 'link' ) {
return renderLinkFormat( { format, item, children: acc } );
}
if ( format.type === 'date' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the this a bit confusing that we want to render text and internally we end up rendering date.

return renderDateFormat( { format, item, children: acc } );
}
return acc;
}, value );

const prefixFormat = field.formats.find(
( format ) => format.type === 'prefix'
);
if ( prefixFormat ) {
return renderPrefixFormat( {
format: prefixFormat,
item,
children: result,
} );
}

const afterFormat = field.formats.find(
( format ) => format.type === 'after'
);
if ( afterFormat ) {
return (
<VStack spacing={ 1 }>
{ result }
{ renderAfterFormat( { format: afterFormat, item } ) }
</VStack>
);
}
return result;
};

export const renderEnumeration = renderText;
7 changes: 6 additions & 1 deletion packages/dataviews/src/view-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ export default function ViewList( {
const primaryField = fields.find(
( field ) => field.id === view.layout.primaryField
);
primaryField.formats = primaryField.formats.filter(
( format ) => format.type !== 'link'
);
const visibleFields = fields.filter(
( field ) =>
! view.hiddenFields.includes( field.id ) &&
Expand Down Expand Up @@ -72,7 +75,9 @@ export default function ViewList( {
</div>
<HStack>
<VStack spacing={ 1 }>
{ primaryField?.render( { item } ) }
<span>
{ primaryField?.render( { item } ) }
</span>
<div className="dataviews-list-view__fields">
{ visibleFields.map( ( field ) => {
return (
Expand Down
Loading
Loading