[DataViews]: Update the view config to include fields visibility #55247
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
youknowriad
reviewed
Oct 11, 2023
ntsekouras
commented
Oct 11, 2023
youknowriad
reviewed
Oct 11, 2023
youknowriad
reviewed
Oct 11, 2023
| @@ -31,6 +32,12 @@ export default function PagePages() { | |||
| field: 'date', | |||
| direction: 'desc', | |||
| }, | |||
| fields: { | |||
| // All fields are visible by default, so it's | |||
| // better to keep track of the hidden ones. | |||
There was a problem hiding this comment.
I'm not sure this assumption is true for all the views. I'd think that it's probably the opposite: most fields should be hidden by default but I'm ok starting this way.
|
Size Change: +168 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Oct 11, 2023
oandregal
reviewed
Oct 11, 2023
| ); | ||
| return <time>{ formattedDate }</time>; | ||
| }, | ||
| enableSorting: false, |
There was a problem hiding this comment.
This field is used to sort the table by default. Why a user shouldn't be able to?
There was a problem hiding this comment.
Maybe a user can do it in the future, but it might be more convoluted when we have composite fields or something that would need to provide the sorting args. I think more of the APIs will evolve/created per iteration and use case.
oandregal
reviewed
Oct 11, 2023
| { | ||
| header: 'Date', | ||
| id: 'date', | ||
| cell: ( props ) => { |
There was a problem hiding this comment.
Following up this rationale, what do you think of doing this instead (pseudo-code):
accessorFn: ( page ) => {
return dateI18n(
getSettings().formats.datetimeAbbreviated,
getDate( props.row.original.date )
);
}
cell: ( props ) => { return <time>{props.row.getValue()}</time> }There was a problem hiding this comment.
I think we need to redo accessorFn and cell to be less tied to the API of tanstack at some point, and yeah we need to agree on a given way of retrieving the fields data.
There was a problem hiding this comment.
I'd echo Riad on this. We need to start forming the API that will map internally to tanstack.
ellatrix
reviewed
Oct 11, 2023
| cell: ( props ) => { | ||
| const formattedDate = dateI18n( | ||
| getSettings().formats.datetimeAbbreviated, | ||
| getDate( props.row.original.date ) |
There was a problem hiding this comment.
The default edit.php page seems to fallback to the modified data for drafts
There was a problem hiding this comment.
The date field in this PR was just a placeholder without the proper handling of this field to showcase the API. We can address this in follow ups.
|
I'm landing this and let's address small minor improvements in follow ups. |
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 updates the passed view config in DataViews component to handle the fields visibility.
Testing Instructions
datefield which is not visible by default.