Skip to content

feat(combobox): add selectAll toggle property #11721

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

Merged
merged 80 commits into from
May 13, 2025

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Mar 8, 2025

Related Issue: #2311

Summary

Provides a Select All checkbox in the calcite-combobox by default for selection-mode="multiple" as a convenient shortcut for users to quickly select or deselect all items in the list. The indeterminate state for this checkbox is a visual and programmatic state that indicates a checkbox is neither fully checked nor unchecked. It represents a "partial selection" in scenario where a checkbox is associated with a group of child checkboxes, and some (but not all) of the child checkboxes are selected.

@Elijbet Elijbet marked this pull request as draft March 8, 2025 01:20
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Mar 8, 2025
@Elijbet Elijbet changed the title feat(combobox): add select-all toggle feat(combobox): add selectAll toggle property Mar 8, 2025
Copy link
Contributor

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 github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Mar 18, 2025
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Mar 19, 2025
Copy link
Contributor

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 github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Mar 27, 2025
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Apr 5, 2025
Copy link
Contributor

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 github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Apr 12, 2025
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Apr 18, 2025
@Elijbet
Copy link
Contributor Author

Elijbet commented May 11, 2025

This is working on my end. Maybe @driskull can test also

I don't see a chip when all items are selected either.

image

It was showing on my end because I didn't have the placeholder set. I made some changes to show it.
newCompactBreakpoint is calculated with getTextWidth, which accounts for the placeholder.

@Elijbet Elijbet 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 May 11, 2025
@Elijbet Elijbet 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 May 12, 2025
@ashetland
Copy link
Contributor

LGTM

@Elijbet Elijbet 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 May 12, 2025
@Elijbet Elijbet merged commit 7dab04f into dev May 13, 2025
14 checks passed
@Elijbet Elijbet deleted the elijbet/2311-select-all-prop-combobox branch May 13, 2025 00:03
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.

Awesome job, @Elijbet! Had a few follow-up review comments, but this looks great!

@@ -1090,6 +1177,10 @@ export class Combobox
connectFloatingUI(this);
}

private setSelectAllComboboxItemReferenceEl(el: HTMLCalciteComboboxItemElement): void {
this.selectAllComboboxItemReferenceEl = el;
Copy link
Member

Choose a reason for hiding this comment

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

Could you look into using a Ref instead? It'd help simplify some code.


it("should toggle indeterminate state to `All Selected` when list items are toggled with a click", async () => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
await testToggleListItems(page, async ([listItem, _combobox]) => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not disable this rule. You don't need to specify _combobox if it's not being used:

 await testToggleAllItems(page, async ([selectAll]) => { /* ... */ });

Applies to similar unused var lines.

Copy link
Contributor Author

@Elijbet Elijbet May 20, 2025

Choose a reason for hiding this comment

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

I'm passing both because I'm using either/or, right? I have 2 it blocks that call testToggleListItems, to test the click I need the listItem, to test the Enter key, I need the combobox.

});

it("should toggle indeterminate state to `All Selected` when list items are toggled with a keydown `Enter`", async () => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

Let's not disable this rule. You don't need to specify _listItem if it's not being used.

 await testToggleAllItems(page, async ([, combobox]) => { /* ... */ });

Applies to similar unused var lines.

const containerRect = listContainer.getBoundingClientRect();

if (itemRect.top < containerRect.top + stickyHeight) {
listContainer.scrollTop -= containerRect.top + stickyHeight - itemRect.top;
Copy link
Member

Choose a reason for hiding this comment

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

Based on the pros that @Elijbet shared, I think it's fine to keep as is. There might be some cases where the z-index could cause some issues, but we can wait until we get feedback.

Ideally, it should be moved out because it doesn't need to be applied as sticky since it never scrolls at all.

Wouldn't it scroll when maxItems is set (see example)?

benelan pushed a commit that referenced this pull request May 14, 2025
**Related Issue:** #2311

## Summary
Provides a `Select All` checkbox in the `calcite-combobox` by default
for `selection-mode="multiple"` as a convenient shortcut for users to
quickly select or deselect all items in the list. The `indeterminate`
state for this checkbox is a visual and programmatic state that
indicates a checkbox is neither fully checked nor unchecked. It
represents a "partial selection" in scenario where a checkbox is
associated with a group of child checkboxes, and some (but not all) of
the child checkboxes are selected.

---------

Co-authored-by: Matt Driscoll <[email protected]>
benelan added a commit that referenced this pull request May 15, 2025
* origin/dev: (277 commits)
  docs(tokens): consistency pass for new component descriptions (#12148)
  build(preset): use valid TS module resolution (#12151)
  docs: update list of contributors (#12134)
  chore: drop obsolete transforms (#12136)
  chore: release main (#11890) (#12147)
  build(deps): bump @arcgis/lumina, typescript, vite, vitest (#12137)
  chore(preset): fix JSON import in build config (#12142)
  docs(panel, action): update `text-color-pressed` token descriptions (#12140)
  chore: release next
  fix(input-time-picker): invert text color on Windows when each masked input is focused (#12130)
  chore: release next
  refactor(sematic-tokens): update `--calcite-corner-radius-default` to reference correct token (#12131)
  feat(semantic-tokens): add `--calcite-color-text-highlight` tokens (#12068)
  chore: release next
  feat(combobox): add `selectAll` toggle property (#11721)
  chore: release next
  fix(sort-handle): allow move and reorder events to be cancelled (#12095)
  chore: release next
  feat: added spike data, heart chart, and progress bar icons (#12127)
  feat(text-area): Add design tokens for corner radius, shadow, footer background color (#12124)
  ...
Elijbet added a commit that referenced this pull request May 27, 2025
**Related Issue:** #11721, #2311

## Summary
Refactor the `combobox` `Select All` toggle logic.
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. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants