Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(filter): adds ability to match only specific filter data properties #9541

Merged
merged 5 commits into from
Jun 14, 2024
Merged

feat(filter): adds ability to match only specific filter data properties #9541

merged 5 commits into from
Jun 14, 2024

Conversation

driskull
Copy link
Member

@driskull driskull commented Jun 7, 2024 •
edited
Loading

Related Issue: #5063

Summary

  • add filterProps argument to filter utility
    • allows filtering fields at the first level of an object.
    • Nested object fields will not be filtered.
      • Could be enhanced to do so in the future by enhancing matchFields to be an object
  • add filter utility spec tests (thank you co-pilot)
  • add filterProps property to filter component
  • add e2e tests for filter

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Jun 7, 2024
@driskull driskull marked this pull request as ready for review June 7, 2024 23:51
@driskull driskull requested a review from a team as a code owner June 7, 2024 23:51
@benelan benelan changed the base branch from main to dev June 10, 2024 09:06
@driskull driskull changed the title feat(list, filter): allow users to choose to match or ignore specific item properties feat(list, filter): adds ability to match or ignore specific filter data properties Jun 11, 2024
@driskull driskull changed the title feat(list, filter): adds ability to match or ignore specific filter data properties feat(list, filter): adds ability to match only specific filter data properties Jun 11, 2024
@driskull
Copy link
Member Author

@jcfranco do you want to review this one or is it good to go?

jcfranco
jcfranco reviewed Jun 12, 2024
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one concern regarding the change in list filtering behavior, but otherwise, this looks great, @driskull!

packages/calcite-components/src/components/filter/filter.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/list/list.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/list/list.tsx Outdated
@@ -558,6 +558,7 @@ export class List
aria-label={filterPlaceholder}
disabled={disabled}
items={dataForFilter}
matchFields={["label", "description", "metadata"]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this also needs to include value. Otherwise, we're changing existing filtering behavior.

Copy link
Member Author

@driskull driskull Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider this a bug. It was never intended to search by value. Value is more of a unique identifier and not content like label, description and metadata are.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For filtering, we should only be searching content. I wouldn't consider value to be content. I think we should not search it by default for anyone using a list with a filter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about I move the list changes to a separate followup PR with the title fix(list): only filter by list item content? @jcfranco

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider this a bug. It was never intended to search by value. Value is more of a unique identifier and not content like label, description and metadata are.

I disagree with this. The filtering has always included value props by default. Could we introduce a matchFields prop to all filtering components, allowing users to omit value if they prefer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcfranco I reverted the list changes in this PR. Can we discuss it as part of #9569? I can create a separate issue for adding matchFields to list if we decide to do that.

Is this PR good to merge?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Will give this another look.

packages/calcite-components/src/utils/filter.spec.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/filter.spec.ts Show resolved Hide resolved
packages/calcite-components/src/utils/filter.spec.ts Show resolved Hide resolved
packages/calcite-components/src/utils/filter.spec.ts Show resolved Hide resolved
packages/calcite-components/src/utils/filter.spec.ts Show resolved Hide resolved
packages/calcite-components/src/utils/filter.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/filter.ts Show resolved Hide resolved
@driskull driskull requested a review from jcfranco June 12, 2024 21:22
@driskull driskull changed the title feat(list, filter): adds ability to match only specific filter data properties feat(filter): adds ability to match only specific filter data properties Jun 12, 2024
jcfranco
jcfranco approved these changes Jun 14, 2024
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️
🕵️🔍🕵️🕵️🕵️🔍🔍🔍🔍🕵️🔍🔍🔍🔍🔍🕵️🔍🕵️🕵️🕵️🔍🕵️🔍🕵️
🕵️🔍🕵️🕵️🕵️🔍🕵️🕵️🕵️🕵️🕵️🕵️🔍🕵️🕵️🕵️🔍🔍🕵️🔍🔍🕵️🔍🕵️
🕵️🔍🕵️🕵️🕵️🔍🕵️🔍🔍🕵️🕵️🕵️🔍🕵️🕵️🕵️🔍🕵️🔍🕵️🔍🕵️🔍🕵️
🕵️🔍🕵️🕵️🕵️🔍🕵️🕵️🔍🕵️🕵️🕵️🔍🕵️🕵️🕵️🔍🕵️🕵️🕵️🔍🕵️🕵️🕵️
🕵️🔍🔍🔍🕵️🔍🔍🔍🔍🕵️🕵️🕵️🔍🕵️🕵️🕵️🔍🕵️🕵️🕵️🔍🕵️🔍🕵️
🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️🕵️

@driskull driskull added the skip visual snapshots Pull requests that do not need visual regression testing. label Jun 14, 2024
@driskull driskull merged commit 137d9ae into dev Jun 14, 2024
11 checks passed
@driskull driskull deleted the dris0000/enhance-filter-matchFields branch June 14, 2024 17:40
@github-actions github-actions bot added this to the 2024-06-25 - Jun Release milestone Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DitwanP DitwanP DitwanP left review comments

@jcfranco jcfranco jcfranco approved these changes

@alisonailea alisonailea alisonailea approved these changes

Assignees

@driskull driskull

Labels
enhancement Issues tied to a new feature or request. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Milestone
2024-06-25 - Jun Release
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@driskull @jcfranco @alisonailea @DitwanP