fix(list): include groups in filtering #10664

anveshmekala · 2024-10-31T21:46:56Z

Related Issue: #7702

Summary

Include calcite-list-item-group in filtering.
group's heading can be used as filterProp.

github-actions · 2024-11-14T01:58:06Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

github-actions · 2024-11-28T02:03:31Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

github-actions · 2024-12-10T02:05:52Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

driskull

Looking good @anveshmekala but have some feedback on implementation.

driskull · 2024-12-20T22:59:10Z

packages/calcite-components/src/components/list/list.tsx

+   * @readonly
+   * @deprecated Use `filteredResults` instead.
+   */
+  @property() filteredItems: ListElement[] = [];


@anveshmekala can we leave filteredItems? Do we need to have a property that includes groups?

since it is a short notice, its better to deprecate first. To avoid confusing with naming, filteredResults is added.

If we are including groups in filter, the user should be able to access the groups me thinks.

Part of my reasoning for not including groups is that they aren't results. They can't be selected, they don't have a value, etc.

Agree item-group is more of a presentational component, but using heading as filterProp can affect how filtering is done. And users should be able to lookup the API for any potential matches for filterText.

It will make it easier if we don't include item-group in filteredItems but filteredItems can't be empty if there is a match.

Did think of adding direct children items of the item-group as filteredItems instead of the item-group but that can be misleading and not consistent with how items are added to filteredItems when item is nested inside a item without a group. Thoughts?

After reviewing the issue again, I think the request was primarily visual and not related to filteredItems. My earlier context was minimal, so apologies for any confusion. 😅

I agree on including them when they are valueless is a bit odd.

Could we proceed with the behavior while keeping filteredItems as is?

driskull · 2024-12-20T23:03:25Z

packages/calcite-components/src/components/list/list.tsx

@@ -160,7 +171,7 @@ export class List

  @state() assistiveText: string;

-  @state() dataForFilter: ItemData = [];
+  @state() dataForFilter: (ItemData | ItemGroupData)[] = [];


Instead of including separate group data, can we just add the group data to each item within a group? It would just be the heading property added to each item.

If we just added group heading to the itemData, then we wouldn't need the logic in filterGroupItems

Theres already existing logic to show a group if one or more items match and hide a group if not. The only part missing is the group data attached to each item.

The above wouldn't work where only group heading is matched (nested group) but none of the items or ancestor group have a matching text.

@anveshmekala it should work.

The current filtering only applies to items, but should also include groups and any matches should show all items under a matching group.

any matches should show all items under a matching group

According to the request, if a group matches, all items under that group should also match.

Its working as expected! 🎉

driskull · 2024-12-20T23:12:12Z

packages/calcite-components/src/components/list/list.tsx

-            {filteredItems.map((item) => (
-              <li>{item.label}</li>
+            {filteredResults.map((item) => (
+              <li role={(item as ListItemGroup).heading ? "group" : "none"}>


For proper semantic structure, groups should be rendered as <ul> or <ol> and no roles would need to be set here. Setting the role to none is taking away from the list role which is automatically applied to <li> elements.

The structure should be like:

<ol> <li>Group 1 <ol> <li>item within group</li> <li>item within group</li> </ol> </li>

Agree, the role attribute wouldn't make a diff how AT handles the information. Idea was to provide the context about a possible group heading matched & semantic structure is not the right fit.

My point was that if you set the role to none on a list-item it takes away the default list-item role.

driskull · 2024-12-20T23:13:40Z

packages/calcite-components/src/components/list/list.tsx

@@ -744,7 +761,7 @@ export class List
    const filteredItems = filterPredicate


The way this is currently structured, filterPredicate would only affect items and not groups.

Meaning to ask, should the filterPredicate include groups? It wasn't mentioned here #3454 but (#7702) was created later.

I don't think it should, just wanted to point out the inconsistency with treating groups like items.

If not, is the alternative metadata + filterProps? While functional, it introduces boilerplate for a common filtering workflow using information already present in the list.

driskull

Getting closer @anveshmekala! 👍

driskull · 2024-12-31T00:57:07Z

packages/calcite-components/src/components/list/list.tsx

    return this.listItems.map((item) => ({
      label: item.label,
      description: item.description,
      metadata: item.metadata,
+      heading: item.parentElement?.closest(listItemGroupSelector)?.heading || "",


Can we store the parentGroup elements on connectedCallback? That way its only ever queried in the DOM one time. In this scenario its queried every time getItemData() is called.

The other part is that this is only getting the first parent group element. Shouldn't it get all ancestor group elements and map to their headings?

The other part is that this is only getting the first parent group element. Shouldn't it get all ancestor group elements and map to their headings?

Good catch. Each item should store the heading of all ancestors group elements.

anveshmekala · 2024-12-31T19:16:18Z

packages/calcite-components/src/components/list/list.tsx

+    let heading = "";
+    this.listItemGroups.forEach((group) => {
+      if (group.contains(item)) {
+        heading += group.heading;


We could also store heading of ancestor as Array object.

Leaning towards string concatenation for performance gains with filter method.

Yeah I was thinking array because string concat would match two words from different headings combined. Can we change to use an array here?

driskull · 2024-12-31T19:25:10Z

packages/calcite-components/src/components/list/list.tsx

+    let heading = "";
+    this.listItemGroups.forEach((group) => {
+      if (group.contains(item)) {
+        heading += group.heading;


Yeah I was thinking array because string concat would match two words from different headings combined. Can we change to use an array here?

driskull · 2024-12-31T19:30:03Z

packages/calcite-components/src/components/list/list.tsx

@@ -649,6 +653,10 @@ export class List
    }
  }

+  private setListItemGroups(): void {
+    this.listItemGroups = Array.from(this.el.querySelectorAll(listItemGroupSelector));


The other alternative would be something like this:

let ancestor = el.closest("calcite-list-item-group"); const headings: string[] = []; while (ancestor) { headings.push(ancestor.heading); ancestor = ancestor.closest("calcite-list-item-group"); }

I'm not sure which would be more performant, calling closest() a few times on connectedCallback or calling setListItemGroups on each mutation. Thoughts @jcfranco?

driskull

This looks good to me 👍

The querying of groups comment is pretty minor and would require storing that information on each item so what you currently have seems to make the most sense.

fix(list): include groups in filtering

c679a94

github-actions bot assigned anveshmekala Oct 31, 2024

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Oct 31, 2024

anveshmekala and others added 5 commits October 31, 2024 21:46

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

becaffd

resolve merge conflicts

053a27f

resolve merge conflicts in e2e

1f02133

fix test failures

85a0e4e

fix filteredItems prop

204f9e4

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 14, 2024

anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Nov 14, 2024

anveshmekala added 4 commits November 18, 2024 13:59

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

363863a

refactor code to lit

c36591c

linting

9c93432

deprecate filteredItems and add filteredResults

cd3c545

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 28, 2024

anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Dec 2, 2024

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Dec 10, 2024

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

246de6f

github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Dec 13, 2024

anveshmekala and others added 8 commits December 18, 2024 15:08

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

15cd55d

refactor

d6f7c24

include heading in filterProps

8ea0e96

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

977e982

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

52dd50c

clean up

56cad5f

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

59a933e

remove console.log

beebb86

anveshmekala requested review from geospatialem, jcfranco, benelan and Elijbet December 20, 2024 21:26

anveshmekala marked this pull request as ready for review December 20, 2024 21:27

anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Dec 20, 2024

driskull requested changes Dec 20, 2024

View reviewed changes

anveshmekala added 5 commits December 20, 2024 18:08

fix a11y test

bdd2543

drop filteredResults & do not include groups in filteredItems

23cd88d

cleanup

4bfefb6

revert doc update

be935d9

update types

40d2c6f

anveshmekala requested a review from driskull December 31, 2024 00:28

handle edge case when visible items are empty

e107542

driskull reviewed Dec 31, 2024

View reviewed changes

anveshmekala and others added 2 commits December 31, 2024 13:59

feedback changes

cab883a

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

bbfbc6b

anveshmekala commented Dec 31, 2024

View reviewed changes

driskull reviewed Dec 31, 2024

View reviewed changes

store heading as array

723d02c

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Dec 31, 2024

driskull approved these changes Dec 31, 2024

View reviewed changes

Merge branch 'dev' into anveshmekala/7702-fix-list-filtering-groups

adef085

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 2, 2025

anveshmekala requested a review from ashetland January 2, 2025 17:16

anveshmekala merged commit 904623b into dev Jan 2, 2025
16 checks passed

anveshmekala deleted the anveshmekala/7702-fix-list-filtering-groups branch January 2, 2025 18:36

github-actions bot added this to the 2025-01-30 - 3.0.0 Jan Breaking Change Release milestone Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(list): include groups in filtering #10664

fix(list): include groups in filtering #10664

anveshmekala commented Oct 31, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 28, 2024

github-actions bot commented Dec 10, 2024

driskull left a comment

driskull Dec 20, 2024

anveshmekala Dec 20, 2024

driskull Dec 20, 2024

anveshmekala Dec 23, 2024 •

edited

Loading

anveshmekala Dec 23, 2024

jcfranco Dec 23, 2024

driskull Dec 20, 2024

anveshmekala Dec 20, 2024

driskull Dec 21, 2024

anveshmekala Dec 23, 2024

driskull Dec 20, 2024

anveshmekala Dec 21, 2024

driskull Dec 21, 2024

driskull Dec 20, 2024

anveshmekala Dec 21, 2024

driskull Dec 21, 2024

jcfranco Dec 23, 2024

driskull left a comment

driskull Dec 31, 2024

anveshmekala Dec 31, 2024

anveshmekala Dec 31, 2024

driskull Dec 31, 2024

driskull Dec 31, 2024

driskull Dec 31, 2024

driskull left a comment

		@@ -744,7 +761,7 @@ export class List
		const filteredItems = filterPredicate

fix(list): include groups in filtering #10664

fix(list): include groups in filtering #10664

Conversation

anveshmekala commented Oct 31, 2024 • edited Loading

Summary

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 28, 2024

github-actions bot commented Dec 10, 2024

driskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anveshmekala Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driskull left a comment

Choose a reason for hiding this comment

anveshmekala commented Oct 31, 2024 •

edited

Loading

anveshmekala Dec 23, 2024 •

edited

Loading