Dataviews: Add some client side data handling utils #57488
Merged
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] Experimental
ntsekouras
requested review from
mcsf,
youknowriad,
oandregal and
jorgefilipecosta
|
Size Change: +607 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jan 2, 2024
packages/dataviews/src/utils.js
Outdated
| * | ||
| * @return {Object[]} Sorted items. | ||
| */ | ||
| export const sortByTextFields = ( { items, view, fields, textFields } ) => { |
There was a problem hiding this comment.
I don't understand why we need the text fields argument, isn't the sorting config part of the "view" object
There was a problem hiding this comment.
For me it seems the type of sorting is related to the field, but if we have an API prop in fields it will only be used for client side(that's what Tanstack does too - sortingFn). So I don't think we need to add such an API like that for now.. Did you have something different in your mind?
There was a problem hiding this comment.
The ideal scenario for me is that the component end up with a single hook abstracting all of this at some point:
// probably a bad name
const { isLoading, data, paginationInfo } = useLocalRequest( allData, view, fields )
separating textFields means that we have to pass an extra argument to such hooks. Isn't the "type" of the field sufficient here?
There was a problem hiding this comment.
Hm.. I'm not sure yet if we could use just the type. For example in templates author the type is enumeration and is used for filters and it could be used for inline editing. But for sorting we just want to do it as a text field.
I think things will become clearer when we advance the other APIs to use the type in more complex scenarios too.
There was a problem hiding this comment.
isn't "enumeration" a special text field?
There was a problem hiding this comment.
Maybe yes, but I'm not too eager to abstract right now because as I said, I think we will be able to make more informed decisions when we evolve the other APIs too.. While not pretty right now, we just pass an array and will be much simpler to change in the near future.
youknowriad
reviewed
Jan 2, 2024
youknowriad
reviewed
Jan 2, 2024
ntsekouras
commented
Jan 3, 2024
| @@ -229,7 +241,12 @@ export default function DataviewsPatterns() { | |||
| ], | |||
| [ view.type, categoryId ] | |||
| ); | |||
|
|
|||
| // Reset the page number when the category changes. | |||
| useEffect( () => { | |||
There was a problem hiding this comment.
This is an unrelated fix for a bug that I observed here.
youknowriad
reviewed
Jan 3, 2024
| // Reset the page number when the category changes. | ||
| useEffect( () => { | ||
| if ( previousCategoryId !== categoryId && view.page !== 1 ) { | ||
| setView( { ...view, page: 1 } ); |
There was a problem hiding this comment.
Right now, these categories are built as separate pages, so for me we should reset the entire "view" object when changing categories.
There was a problem hiding this comment.
I kind of disagree here because while what you say it's true, it feels like we have external filters in the sidebar and for example if I have sorted by title, I'd expect the other categories to be sorted too. No strong opinions for now though, I can update.
There was a problem hiding this comment.
No strong opinion too. I don't like this effect though and I'm not sure how to avoid it.
There was a problem hiding this comment.
In my mind this is just a filter that we will need to incorporate in the view object. Current patterns page is WIP and has many more things to think about - refactor/simplify etc.. It's just that is there is a lot of complexity and want to tackle things iteratively.
There was a problem hiding this comment.
This is also close to the "default views" of the pages data views, it's not really clear whether it's filters or just separate default views.
There was a problem hiding this comment.
This is also close to the "default views" of the pages data views, it's not really clear whether it's filters or just separate default views.
The "default views" are just presets, preconfigured settings for the view, so we still need a "category" filter in place.
youknowriad
reviewed
Jan 3, 2024
| @@ -252,10 +256,10 @@ export default function DataviewsTemplates() { | |||
| [ authors, view.type ] | |||
| ); | |||
|
|
|||
| const { shownTemplates, paginationInfo } = useMemo( () => { | |||
| const { data, paginationInfo } = useMemo( () => { | |||
There was a problem hiding this comment.
Can this hook become a reusable hook that filters and sorts and everything based on "view", "allData" and "fields"?
There was a problem hiding this comment.
We can explore this separately better because I think we need to advance the filters API more and see how we can abstract it best. Right now there is a lot of specific code regarding the filtering(see author in templates).
There was a problem hiding this comment.
It doesn't seem specific at all for me. author_text is just getValue and the rest is standard. I'm fine if it's explored separately but I think having something like this is going to help us make the right decisions. And we won't need to expose multiple helpers from the dataviews package only a single hook.
There was a problem hiding this comment.
Of course, eventually we shouldn't have much helpers..
youknowriad
approved these changes
Jan 3, 2024
ntsekouras
deleted the
dataviews/add-some-client-side-data-handling-utils
branch
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?
Follow up of: #57333
When data handling is done client side there is some code that can be abstracted and reused. In this PR I introduce two of them:
Testing Instructions