Data Array Enhancement (followup) #6725
Conversation
src/js/components/Data/filter.js
Outdated
| if (Array.isArray(value)) { | ||
| return !filterValue.some((f) => | ||
| typeof f === 'object' | ||
| ? value.includes(f.value) // from {label: ___, value: ___} |
There was a problem hiding this comment.
It looks like previous we were doing a "filterValue.includes(value)" pattern and now we're doing "value.includes(filter)" -- can you explain why we made this switch? Was that a bug in the initial implementation?
There was a problem hiding this comment.
Connected over call - value.includes(filter) is used in this case because we iterate over filterValue and we use typeof f to determine if each f is in the form of an object {label: ___, value: __}. If so, we want to check whether or not value includes f.value. Previously, there was no check for typeof f, which resulted in incorrect behavior when each value was a {label: __, value: __} object.
| @@ -76,7 +85,8 @@ export const Complex = () => ( | |||
| status="info" | |||
| message="Data is in 'beta'. The API surface is subject to change." | |||
| /> | |||
| <Data data={data} properties={properties} toolbar> | |||
| <Data data={data} properties={properties} updateOn="change"> | |||
| <DataFilters layer /> | |||
There was a problem hiding this comment.
One follow up, is it necessary to change the storybook layout of can we leave as it was?
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm fine adjusting this story to the DataFilters layer, but I'd probably recommend we don't add "updateOn=change" in that case.
There was a problem hiding this comment.
Thanks for clarifying! Will make that final edit.
|
4/14/23 status: Final stages of review, waiting upon final review to wrap things up and should be merged today or early on next week. |
What does this PR do?
follow up from #6704, fixes filtering when filter value is an object with {label: __, value: __}
Where should the reviewer start?
src/js/components/Data/filter.js
src/js/components/Data/stories/Complex.js
What testing has been done on this PR?
manual + visual testing
How should this be manually tested?
ensure that filtering works as intended
Do Jest tests follow these best practices?
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Should this PR be mentioned in the release notes?
Is this change backwards compatible or is it a breaking change?