-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
63ac289
aeb413c
4bb8d92
8eccbe3
e553f2b
934eb7b
4f3cefa
e8abd11
c15853c
a18363a
0523e32
1123df1
0bf5a2f
5778d1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,25 +150,16 @@ Example: | |
[ | ||
{ | ||
id: 'date', | ||
type: 'date', | ||
header: __( 'Date' ), | ||
getValue: ( { item } ) => item.date, | ||
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' } | ||
|
@@ -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. | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I've shared the current issue we have in the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of consumers overriding the Sharing some issues off the top of my head to illustrate the point:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This could be a different separate API and I agree this should be handled internally. Maybe if we do make the A similar problem we already have is how to determine if we need to render a placeholder in
What is the problem with labels? And how it's solved with formats? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for sharing, the list of issues is growing :)
See comment by Andrew. Fields other than the primary need to point to the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is very similar to what the There's also a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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 || []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should do something like |
||
|
||
// Normalize render. | ||
switch ( field.type ) { | ||
case DATE_TYPE: | ||
field.render = ( { item } ) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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"> | ||
|
@@ -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 } | ||
|
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"> | ||
— | ||
</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' ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the this a bit confusing that we want to render |
||
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; |
There was a problem hiding this comment.
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 ]
There was a problem hiding this comment.
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 sametype
. This kind of shortcut would work for just a handful of cases(only the field types we support).There was a problem hiding this comment.
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.