-
Notifications
You must be signed in to change notification settings - Fork 4.5k
DataViews: Allow searching over array fieldtypes #70785
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
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: +22 B (0%) Total Size: 1.89 MB
ℹ️ View Unchanged
|
Searching over array fieldtypes with `enableGlobalSearch: true` threw a TypeError because the `normalizeSearchInput` function expected a string but received an array. This change fixes that by treating every value as an array and adapting the logic to match, before entering `normalizeSearchInput`.
This shows off that the search works over array fields too.
Also changed the second test so it's not just returning all results so it's clear that it is actually searching.
ca9f8e4
to
1dfdec6
Compare
Flaky tests detected in 46a8d4b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16464801606
|
const { data: result } = filterSortAndPaginate( | ||
data, | ||
{ | ||
search: 'Moon', |
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.
title
and description
also have global search enabled, so this can give false positives because the 3 items (Europa, Io, Moon) have "moon" in the title or description. Perhaps change the search to Satellite
?
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 for the fix.
I'm pre-approving in the interest of velocity, though I think it'd be better to update the tests search string (see).
… on title / description
Whoops, forgot to copy the props into the commit message 😬, sorry about that. |
Ah, I forgot to mention something: this needs a changelog update :) |
Searching over array fieldtypes with `enableGlobalSearch: true` threw a TypeError because the `normalizeSearchInput` function expected a string but received an array. This change fixes that by treating every value as an array and adapting the logic to match, before entering `normalizeSearchInput`.
It was missed when merging WordPress#70785 originally. Co-authored-by: dsas <dsas@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
What?
Closes #70782
Fixing a bug with searching over DataView fields that have the array field types.
Also updating the DataViews storybook to demonstrate that it works.
Why?
This fixes a bug with searching over DataView fields that have the array fieldtype. Previously they would crash with TypeError: input.trim is not a function
How?
Treats every value as an array before normalising and searching over every value in that array.
Testing Instructions
I've added some tests, to test manually:
Testing Instructions for Keyboard