-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 a utility to share filtering, sorting and pagination logic #59897
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -394 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
const fieldToSort = _fields.find( ( field ) => { | ||
return field.id === fieldId; | ||
} ); | ||
filteredData.sort( ( a, b ) => { |
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.
What about sortable fields that are not text(ex Date)? Maybe the field
could provide an override sort function and we could maintain some defaults. For example in fields
API:
sort: 'date', and we could pick up a date sorter. Of course the could provide a function.
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.
Yes, something like that would be good (especially when we introduce the "field types" more formally.
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.
Thanks Riad! I didn't spot anything weird while testing and I've left some small comments. In general I'd like some input from @oandregal about the filters, but either way is the right direction to land and revisit when more filters(with different types, operators, etc..) are added.
My thinking is that we need to be exhaustive with filters and operators here. Any filter or operator we add need to be supported by this function. So I'd like to support all the current ones before merging this PR, it shouldn't be that hard, it's just unclear to me what things exist and what is their format. |
58a3a63
to
20ce6ed
Compare
Pushed changes to implement the remaining operators. Also had to rebase because there was some other operators defined in |
data: filteredData, | ||
view, | ||
} ); | ||
// Since filters are applied server-side, |
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 is an optimization, as it should work with the filters & search. Not super fan of asking consumers to do this, I'd like it more if:
- offer a config for this all-in-one utility, as in
filterAndSortDataView( data, view, fields, options );
- create micro-utilities (
sort
,filter
,paginate
)
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 don't like micro-utilities personally for this but I'd be open for options. Maybe we can start like today and iterate.
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.
What would you think of adding an options
parameter?
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.
To be honest, that was the first thing I did when I was working on the PR but then I though, why adding an "filter" flag and at the same time having "filters" in view. "view" in a sense is already an options param, so I just deleted the filters.
I guess this is me saying that I'm not sure what's the best. I agree that for us as consumers it's simpler to have an option but if you think about the API itself in isolation, it feels weird to pass "filters" but to tell it not to filter.
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.
Yeah, I see.
Other libraries absorb this complexity (IIRC, tanstack table had configuration for that) but I actually like the simpler data flow we have: data
is received by DataViews
already prepared. If a consumer forgets to remove the operations the worst that will happen is that it becomes a little more slow, probably isn't going to be a big deal. We could add some comments/docs about this use case.
8ac651f
to
982ddff
Compare
|
@oandregal or @youknowriad one of you rebased and removed my commit that made the changes in the filtering, storybook and also had the rename in data for storybook.. |
I don't have anything else to add: the storybook is now working, it has a new filter categories that supports all multi-select operators, tests are there for all operators as well. In terms of the API, my remaining questions are: naming, and how to configure the behaviors consumers want to use. I'd prefer if we land something we consider future-proof so consumers don't have to adapt too much, but these comments are not blocking — it's ok to land and iterate later given the experimental/private nature of this package. |
) { | ||
filteredData = filteredData.filter( ( item ) => { | ||
const fieldValue = field.getValue( { item } ); | ||
if ( Array.isArray( fieldValue ) ) { |
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.
We should rely on the field.type instead of doing this. This is for a follow-up, when we have type string
or array
instead of enumeration
.
Oh, sorry to hear. Based on what you shared and comparing the changes before/after I rebased yesterday afternoon, it must have been me. Sorry about that 🙏 I hope the changes were not too big. If they were, ping me and I'm happy to help you investigate locally to recover those commits. |
import { normalizeFields } from './normalize-fields'; | ||
|
||
function normalizeSearchInput( input = '' ) { | ||
return removeAccents( input.trim().toLowerCase() ); |
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.
Annoying that we still need to use third-party libraries like remove-accent
for this kind of stuff.
Intl.Collator#compare is almost what we want, but it doesn't do partial matches.
</rant>
:)
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 just copied what we had before, now let me merge my PR and do your follow-up :P
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.
Hah, I know, this was just a rant in passing :P
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.
Oh hey, it's @tyxla's package! No disrespect meant! 😅
…ogic (WordPress#59897) Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Related #55083
What?
A lot of data views are "local dataviews", meaning that all the filtering, sorting and pagination logic is done on the client. This PR creates a single utility to share this logic across all of this kind of data views.
Testing Instructions
1- you can test that filtering, sorting, pagination still work as expected in patterns and template parts dataviews.