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

feat(list): add filterProps property to specify which properties to filter against #9622

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

driskull
Copy link
Member

@driskull driskull commented Jun 17, 2024

Related Issue: #9619

Summary

  • add filterProps property to list
  • changes matchFields to filterProps on the filter component.
  • add tests

@driskull driskull marked this pull request as ready for review June 17, 2024 22:48
@driskull driskull requested a review from a team as a code owner June 17, 2024 22:48
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Jun 17, 2024
@driskull driskull added the skip visual snapshots Pull requests that do not need visual regression testing. label Jun 20, 2024
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.

📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝
📝🏑📝📝📝🏑📝📝🏑🏑📝📝🏑🏑🏑📝📝🏑🏑🏑📝🏑🏑🏑🏑📝🏑📝
📝🏑🏑📝📝🏑📝🏑📝📝🏑📝📝🏑📝📝🏑📝📝📝📝🏑📝📝📝📝🏑📝
📝🏑📝🏑📝🏑📝🏑📝📝🏑📝📝🏑📝📝🏑📝📝📝📝🏑🏑🏑📝📝🏑📝
📝🏑📝📝🏑🏑📝🏑📝📝🏑📝📝🏑📝📝🏑📝📝📝📝🏑📝📝📝📝📝📝
📝🏑📝📝📝🏑📝📝🏑🏑📝📝🏑🏑🏑📝📝🏑🏑🏑📝🏑🏑🏑🏑📝🏑📝
📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝📝

const list = await page.find("calcite-list");
list.setProperty("matchFields", ["label", "description"]);
await page.waitForChanges();
await page.waitForTimeout(listDebounceTimeout);
Copy link
Member

Choose a reason for hiding this comment

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

Is there an event we can listen for? This is fine for now, but we should start moving away from timeouts as puppeteer@22.2.0 removed page.waitForTimeout.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not currently, we would need some kind of event for when filteredItems is updated. Can you create an issue to add this so we can clean up these tests?

Copy link
Member

Choose a reason for hiding this comment

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

packages/calcite-components/src/components/list/list.tsx Outdated Show resolved Hide resolved
@@ -160,6 +160,16 @@ export class List
*/
@Prop({ reflect: true }) loading = false;

/**
* Specifies the fields to match against when filtering. If not set, all fields will be matched (label, description, metadata, value).
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, late to the party here - is "fields" an obvious term here for this? "If not set, all fields will be matched (label, description, metadata, value)"

Really it's matching supplied property values? Feel free to disregard but "fields" doesn't immediately convey what will be matched to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

We already called the property matchFields on filter.tsx but we can change before we release if we have a beter name. matchPropertyValues or matchKeyValues? I think its really a key. Could also be matchKeys?

@jcfranco

Copy link
Member

Choose a reason for hiding this comment

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

naming

WDYT about matchProps?

Do we want to also revisit match as well? If we were considering both, filterProps could also be an option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kinda like filterProps. // the properties to use for filtering? @macandcheese? @jcfranco

Copy link
Member Author

Choose a reason for hiding this comment

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

We will likely need to update the changelog at release to modify matchFields to filterProps on the filter component.

@driskull driskull changed the title feat(list): add matchFields property to specify which fields to filter against feat(list): add filterProps property to specify which properties to filter against Jun 21, 2024
@driskull driskull merged commit a253c00 into dev Jun 21, 2024
12 checks passed
@driskull driskull deleted the dris0000/list-add-matchFields branch June 21, 2024 19:55
@github-actions github-actions bot added this to the 2024-06-25 - Jun Release milestone Jun 21, 2024
benelan added a commit that referenced this pull request Jun 24, 2024
…rovements

* origin/dev:
  build(deps): update typescript-eslint monorepo to v7.13.1 (#9657)
  docs: update component READMEs (#9668)
  chore: release next
  chore: minor git tooling tweaks (#9664)
  feat(combobox): add `filterText` prop (#9654)
  chore: release next
  feat(list): add filterProps property to specify which properties to filter against (#9622)
  chore: release next
  fix(list): enable dragging on list items contained within a list that supports dragEnabled (#9660)
  test(filter): add coverage for empty filter text (#9655)
  chore: release next
  fix: set the correct Sortable parent when within a shadowRoot (#9532)
  fix(combobox): allow arrow selection of entered text (#9629)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants