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.

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

fix(list): only filter by list item content #9569

Closed
wants to merge 7 commits into from

Conversation

driskull
Copy link
Member

Related Issue: #5063

Summary

  • set default matchFields for list component
  • add e2e tests

@driskull
Copy link
Member Author

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?

@jcfranco the reason I think this was unintentional is because...

  • I think Ideally, filter should only be filtering by content (a label, description, etc)
  • The things the end user actually sees and is displayed by the component.
    • value is never displayed to the end user which means they may wonder why an item is matching.
    • I don't think we should consider value to be content. In the majority of cases, its a unique identifier.
    • I don't think there's a valid use case for searching by value. the metadata property is for the use case to add more matching terms.
  • By default, it would be nice if users didn't have to configure matchFields on a list to get the best setup.

Thoughts? If you disagree, I will add matchFields to the list component and value will be matched by default.

Base automatically changed from dris0000/enhance-filter-matchFields to dev June 14, 2024 17:40
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jun 14, 2024
@jcfranco
Copy link
Member

This is a great discussion since the original issue only focused on the filter component API.

Thoughts? If you disagree, I will add matchFields to the list component and value will be matched by default.

Let's proceed with this at the moment. I disagree on changing this behavior at this moment. We don't have data to make this call confidently and users have always been able to filter by value.

If we make it a pattern for filter-owning components to define a metadata property for additional search terms to consider (I think combobox is the only pending one), we can revisit not searching value by default in a breaking change release. We should reach out to different teams to get more info on their filtering use cases before doing so.

Does the following match the approach we're going with?

Users use matchFields to define searchable fields on items, where metadata contains additional search criteria to match. Components define which fields are used for searching items along with metadata.

Pros

  • simple to implement
  • easy to follow
  • allows overrides

Cons

  • requires user to set up separate object for searching (minor)
  • requires users to specify each item property when omitting a few props (minor?)

@driskull
Copy link
Member Author

I'll proceed with creating a matchFields on list.

However, I still strongly feel that we should make it a pattern to only be filtering by visible content by default.

value isn't visible anywhere nor content.

@jcfranco
Copy link
Member

However, I still strongly feel that we should make it a pattern to only be filtering by visible content by default.
value isn't visible anywhere nor content.

Won't metadata matches share the same issue?

@driskull
Copy link
Member Author

Won't metadata matches share the same issue?

No because metadata isn't defined by default. My argument is that we shouldn't be filtering using value by default since it is not anything visible to the end user.

@jcfranco
Copy link
Member

Putting the default aspect aside, you have a concern about matching non-visible values being confusing. This could still look/behave the same way to users, right?

@driskull
Copy link
Member Author

Putting the default aspect aside, you have a concern about matching non-visible values being confusing. This could still look/behave the same way to users, right?

No, I don't have any concern about matching non visible content if a user set metadata since that is what that property is for. Its for providing additional matching.

My concern is just about the default experience of matching against value since value is not content. Its an identifier which in most cases isn't search worthy.

I think in more cases than not, a user will need to set matchFields in order to not search value.

Whatever we decide, it will become a pattern because we're also searching value in components like combobox.

I just don't get how searching value by default is good in cases like this:

<calcite-combobox>
  <calcite-combobox-item value="8fd5d99a-100d-4800-8e88-f29a5477c559" text-label="Natural Resources"></calcite-combobox-item>
  <calcite-combobox-item value="3e8f074e-2d88-48bf-b669-22047777a603 " text-label="Agriculture"></calcite-combobox-item>
</calcite-combobox>

It means if someone enters the character "f" or "d" its going to match to both those items for no good reason.

@jcfranco
Copy link
Member

I see your point and that's a great example, but I can also see use cases using the value directly. Like I mentioned above, we can revisit this for a breaking change release and after getting feedback from teams.

@driskull
Copy link
Member Author

I can also see use cases using the value directly.

I don't see this use case as valid. If a user wants to add additional search matching, they can use metadata instead. The purpose of value is for a unique identifier for the item which may be used for form submission or apps/components to identify the selected values.

Like I mentioned above, we can revisit this for a breaking change release and after getting feedback from teams.

Sounds good to me. Doing this during a breaking change would be beneficial.

@driskull driskull closed this Jun 17, 2024
@driskull driskull deleted the dris0000/list-filter-fields branch June 17, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants