DataViews: add new field types and formats to modify the default render
#56942
Conversation
oandregal
added
[Type] Experimental
oandregal
requested review from
youknowriad,
andrewhayward,
jorgefilipecosta and
ntsekouras
|
Size Change: +388 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
| - 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.
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.
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.
There was a problem hiding this comment.
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:
- DataViews Field rendering: Context should be not be assumed by fields when rendering #56320
- DataViews: iterate on list view #56746 (comment)
- Different styles for page & template pages: aka how dataview can provide a coherent experience when we have dozens of consumers?.
There was a problem hiding this comment.
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
linkis provided in a structured data (aformat), 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.?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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).
There was a problem hiding this comment.
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.
| @@ -150,25 +150,16 @@ Example: | |||
| [ | |||
| { | |||
| id: 'date', | |||
| type: 'date', | |||
| header: __( 'Date' ), | |||
| getValue: ( { item } ) => item.date, | |||
There was a problem hiding this comment.
This could be optional too because it's just item[ id ]
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I think it would work in most cases, I'm not talking about reusing the "type", but using the "id" instead.
| } ) ); | ||
| return fields.map( ( field ) => { | ||
| // Normalize formats. | ||
| field.formats = field.formats || []; |
There was a problem hiding this comment.
This looks like a mutation during render, probably something that can cause bugs.
There was a problem hiding this comment.
you should do something like const result = { ...field } and mutate result instead.
| // Normalize render. | ||
| switch ( field.type ) { | ||
| case DATE_TYPE: | ||
| field.render = ( { item } ) => |
There was a problem hiding this comment.
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...
|
Flaky tests detected in 5778d1f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7167373737
|
|
That's interesting! I have been exploring some similar things here, so we can discuss the different approaches. |
| if ( format.type === 'link' ) { | ||
| return renderLinkFormat( { format, item, children: acc } ); | ||
| } | ||
| if ( format.type === 'date' ) { |
There was a problem hiding this comment.
I find the this a bit confusing that we want to render text and internally we end up rendering date.
From what I can tell, they are completely unrelated :) 56945 is about inline editing. This is about whether we could replace the |
|
@youknowriad @ntsekouras I'm interested in understanding whether this PR (or something along these lines) is the way forward to solve the issues we're experiencing with I'm not that invested in the particular implementation, naming, etc. Before looking into implementation deeply, I'd like to understand if we can replace |
|
This was a fun exploration but making the field rendering declarative (at least with this implementation) is not something that has gained support from other people. The current thinking is that consumers should be able to render whatever they want, which means that we should look at solving the issues listed in the PR description differently. |
| ); | ||
| }, | ||
| type: TEXT_TYPE, | ||
| getValue: ( { item } ) => item.title?.rendered, // TODO: see styles for pages without title (link). |
There was a problem hiding this comment.
Extracted this to #57434
| @@ -270,14 +259,8 @@ export default function PagePages() { | |||
| { | |||
| header: __( 'Date' ), | |||
| id: 'date', | |||
| type: DATE_TYPE, | |||
There was a problem hiding this comment.
This could be extracted to a separate PR (rendering dates based on the type) as it's working well. Given it'd be only for this type, I don't see the point of doing it right now.
Part of #55083
What?
This PR is a proof of concept that experiments with replacing the
renderfunction of fields bytypesandformats.Examples:
datefield that uses the default render:textfield that needs links and a custom empty state:Why?
As the API currently stands, the fields'
renderfunction needs to account in which view layout the field would be rendered (see implementations for templates and pages).This approach demands more from the consumer. They need to account for accessibility (labels, empty cells should use
–, etc.), keyboard navigation in different views (do not provide links in list view, for example), or remember to use the same styling in all the pages that use the same view, for example.Some examples of the issues this causes are:
render.How?
text,date) that provide a default render for fields.formats, which are modifiers of the default render provided by the type. Examples are:linkformat. It's up to the view if it uses it or not (e.g.: can be supported intablebut not inlist).prefixformat, which can be used to add icons before the default rendered. It's up to the view if this is supported or not (e.g.: can be supported intablebut not inlist).Testing Instructions
TODO / QUESTIONS