-
Notifications
You must be signed in to change notification settings - Fork 79
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
Conversation
selectAll
toggle property
… persist select from aquiring the checkbox Select all
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. |
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. |
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. |
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
It was showing on my end because I didn't have the placeholder set. I made some changes to show it. |
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
LGTM |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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]) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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)?
**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]>
* 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) ...
Related Issue: #2311
Summary
Provides a
Select All
checkbox in thecalcite-combobox
by default forselection-mode="multiple"
as a convenient shortcut for users to quickly select or deselect all items in the list. Theindeterminate
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.