[Data views]: Refactor to allow the rendering to be generated using a fields description #55444
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Code Quality
ntsekouras
requested review from
mcsf,
youknowriad,
oandregal and
jorgefilipecosta
ntsekouras
commented
Oct 18, 2023
| date: { | ||
| view: ( { item, getValue } ) => <time>{ getValue( { item } ) }</time>, | ||
| }, | ||
| image: { |
There was a problem hiding this comment.
This works for WP images now, where we pass the attachment id. Later we could change name here and also add a field type that gets the src or more props and renders just an img tag.
|
Size Change: +72 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
oandregal
reviewed
Oct 18, 2023
oandregal
reviewed
Oct 18, 2023
… fields description
ntsekouras
force-pushed
the
add/field-rendering-based-on-description
branch
from
1aaa890
to
8c8debe
Compare
ntsekouras
force-pushed
the
add/field-rendering-based-on-description
branch
from
9e58774
to
2f354f3
Compare
oandregal
reviewed
Oct 18, 2023
| } | ||
| /> | ||
| ) : null, | ||
| ...coreFieldTypes.image, |
There was a problem hiding this comment.
Could you clarify why we do this if featured-image defines its own render? (same for the other fields that end up overriding the default render)
There was a problem hiding this comment.
For now it doesn't do more, but the goal is to have more default things like edit. Maybe it's makes more sense to try to see how the edit would be here and evaluate again. I'll convert to draft.
oandregal
reviewed
Oct 18, 2023
| ); | ||
| return <time>{ formattedDate }</time>; | ||
| }, | ||
| enableSorting: false, |
There was a problem hiding this comment.
Suggested change
| enableSorting: false, |
oandregal
reviewed
Oct 18, 2023
| return <time>{ formattedDate }</time>; | ||
| }, | ||
| ...coreFieldTypes.date, | ||
| render: |
There was a problem hiding this comment.
I'm thinking we could update the getValue to:
getValue( { item } ) => dateI18n(
getSettings().formats.datetimeAbbreviated,
getDate( item.date )
)
and then use the default render provided by the core date type.
Or the other way around: make the dateI18n part of the core's type. Does that make sense?
I guess my point is that unless we benefit from introducing the core's types, it's hard to justify?
oandregal
reviewed
Oct 18, 2023
| @@ -167,6 +191,7 @@ export default function PagePages() { | |||
| id: 'status', | |||
| getValue: ( { item } ) => | |||
| postStatuses[ item.status ] ?? item.status, | |||
| ...coreFieldTypes.string, | |||
There was a problem hiding this comment.
So far, this is the only change of this PR, correct? status will now have a render (aka cell) while it didn't before. Presumably because tanstack handled it for us, and now we handle it directly via the core's types.
There was a problem hiding this comment.
Actually, given what we do at packages/edit-site/src/components/dataviews/dataviews.js:
render: field.render?.( field.getValue ) || field.getValue,We don't need this, do we?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of: #55083
This PR introduces some core field types and use them for rendering of fields based on this config. If a fields needs to do more than the default behavior, it can override the prop.
This approach makes the field's
getValueprop, a required one. We need to check later on the nuances of mapping this toaccessorFnforlistview, but that's a separate issue to tackle.The format I'm suggesting is a closure function that first gets the
getValuefunction of a field, and this is needed because we normalize therenderfunction in the mainDataViewscomponent. At that point we have no access to a single data record.In the long run we will add more things to the field types, like an
editthat could be something like:Testing Instructions