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(combobox): add single-persist selection mode #7583

Merged
merged 3 commits into from
Aug 25, 2023

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Aug 23, 2023

Related Issue: #4738

Summary

Adds single-persist mode to calcite-combobox to allow users to opt-in to this change in selection behavior (vs making it a breaking change on the current single selection mode).

clearDisabled would behave the same way unless we want to make an exception to this mode.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Aug 23, 2023
@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Aug 23, 2023
@jcfranco jcfranco marked this pull request as ready for review August 24, 2023 18:07
@jcfranco jcfranco requested a review from a team as a code owner August 24, 2023 18:07
@@ -200,10 +200,11 @@ export class Combobox
* Specifies the selection mode:
* - `multiple` allows any number of selected items (default),
* - `single` allows only one selection,
* - `single-persist` is like single, but does not allow deselecting,
Copy link
Member Author

Choose a reason for hiding this comment

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

+@geospatialem for editorial consideration 📖🔍👁️👄👁️

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

You win the internet, @driskull. 🏆🌐

Copy link
Member

Choose a reason for hiding this comment

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

In accordion we use:

"single-persist" allow and require one open item.

For consistency, WDYT about:

"single-persist" allows and requires one selection,

Copy link
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

Few editorial comments below for consideration.

@@ -85,12 +85,13 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
* Specifies the selection mode:
* - `multiple` allows any number of selected items (default),
* - `single` allows only one selection,
* - `single-persist` is like single, but does not allow deselecting,
Copy link
Member

Choose a reason for hiding this comment

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

In accordion we use:

"single-persist" allow and require one open item.

For consistency, WDYT about:

"single-persist" allows and requires one selection,

@@ -200,10 +200,11 @@ export class Combobox
* Specifies the selection mode:
* - `multiple` allows any number of selected items (default),
* - `single` allows only one selection,
* - `single-persist` is like single, but does not allow deselecting,
Copy link
Member

Choose a reason for hiding this comment

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

In accordion we use:

"single-persist" allow and require one open item.

For consistency, WDYT about:

"single-persist" allows and requires one selection,

Comment on lines 85 to 88
* Specifies the selection mode:
* - `multiple` allows any number of selected items (default),
* - `single` allows only one selection,
* - `single-persist` is like single, but does not allow deselecting,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick:

For consistency, can we add double quotes to each of the values? For instance:

- `"multiple"`....
- `"single"`....
- `"single-persist"`...
- etc.

Comment on lines 201 to 203
* - `multiple` allows any number of selected items (default),
* - `single` allows only one selection,
* - `single-persist` is like single, but does not allow deselecting,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick:

For consistency, can we add double quotes to each of the values? For instance:

- `"multiple"`....
- `"single"`....
- `"single-persist"`...
- etc.

Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

👍 Looks great 💯 🌮 🏆

Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

👍

@@ -1134,7 +1135,7 @@ export class Combobox
}

isMulti(): boolean {
return this.selectionMode !== "single";
return this.selectionMode !== "single" && this.selectionMode !== "single-persist";
Copy link
Member

Choose a reason for hiding this comment

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

Could this be return !isSingleLike(this.selectionMode)?

@@ -1187,7 +1188,7 @@ export class Combobox

renderInput(): VNode {
const { guid, disabled, placeholder, selectionMode, selectedItems, open } = this;
const single = selectionMode === "single";
const single = selectionMode === "single" || selectionMode === "single-persist";
Copy link
Member

Choose a reason for hiding this comment

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

could this be const single = isSingleLike(selectionMode)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catches! 🎣

@@ -1284,7 +1285,7 @@ export class Combobox
const { selectedItems, placeholderIcon, selectionMode, placeholderIconFlipRtl } = this;
const selectedItem = selectedItems[0];
const selectedIcon = selectedItem?.icon;
const singleSelectionMode = selectionMode === "single";
const singleSelectionMode = selectionMode === "single" || selectionMode === "single-persist";
Copy link
Member

Choose a reason for hiding this comment

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

could this be const single = isSingleLike(selectionMode)?

@jcfranco jcfranco 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 Aug 25, 2023
@jcfranco jcfranco merged commit dab06a3 into main Aug 25, 2023
16 checks passed
@jcfranco jcfranco deleted the jcfranco/4738-add-single-persist-mode-to-combobox branch August 25, 2023 20:20
@github-actions github-actions bot added this to the 2023 August Priorities milestone Aug 25, 2023
benelan added a commit that referenced this pull request Aug 25, 2023
…input

* origin/main:
  chore: release next
  fix(tree-item): updates state when selection changes programmatically for `selection-mode='ancestors'` (#7572)
  chore: release next
  feat(combobox): add single-persist selection mode (#7583)
  chore: release next
  fix(modal): catch error when beforeClose promise is rejected (#7600)
  fix(flow): catch error when beforeBack promise is rejected (#7601)
  test(list): Use the dragAndDrop utility for mouse tests (#7604)
Elijbet pushed a commit that referenced this pull request Aug 30, 2023
**Related Issue:** #4738 

## Summary

Adds `single-persist` mode to `calcite-combobox` to allow users to
opt-in to this change in selection behavior (vs making it a breaking
change on the current `single` selection mode).

`clearDisabled` will behave the same way unless we want to make an
exception to this mode.
benelan pushed a commit that referenced this pull request Sep 1, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 1.7.0</summary>

##
[1.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@1.6.1...@esri/calcite-components@1.7.0)
(2023-09-01)


### Features

* **action-bar, action-pad, action-group:** Add label properties for
group context
([#7415](#7415))
([b34f36d](b34f36d))
* **combobox:** Add single-persist selection mode
([#7583](#7583))
([dab06a3](dab06a3))
* **flow:** Add support for custom flow-item elements
([#7608](#7608))
([197adfe](197adfe))
* **input-date-picker:** Normalize year to current century for user
typed values only
([#7638](#7638))
([a1db718](a1db718))
* **input-number:** Add integer property
([#7646](#7646))
([cd66a6d](cd66a6d))
* **input-time-picker:** Support fractional seconds
([#7532](#7532))
([c2bf34b](c2bf34b))
* **meter:** Add Meter component
([#7401](#7401))
([47163ed](47163ed))
* **sheet:** Add Sheet component
([#7561](#7561))
([f12a393](f12a393))
* **sheet:** Update default widths
([#7617](#7617))
([47d2c0b](47d2c0b))
* **shell:** Add "Sheets" Slot
([#7579](#7579))
([e798765](e798765))
* **table:** Add Table and related components
([#7607](#7607))
([b067e72](b067e72))


### Bug Fixes

* **accordion, accordion-item:** Improve a11y
([#7560](#7560))
([b5170b6](b5170b6))
* Add drag styles for improved UX
([#7644](#7644))
([afbb764](afbb764))
* **block, block-section:** Improve a11y
([#7557](#7557))
([1f44f6b](1f44f6b))
* **chip-group:** Add existence checks
([#7586](#7586))
([5ca64f1](5ca64f1))
* **color-picker:** Update value when alphaChannel is toggled
([#7563](#7563))
([1f753dd](1f753dd))
* **combobox:** Prevent deselecting items via keyboard in single-persist
mode
([#7634](#7634))
([4f5f8b0](4f5f8b0))
* **combobox:** Update combobox height to follow design spec
([#7558](#7558))
([ec08845](ec08845))
* **date-picker:** Set start of week to monday in zh-CN
([#7578](#7578))
([7e385cb](7e385cb))
* **dropdown:** Prevents navigating dropdown items with Tab key
([#7527](#7527))
([3ea658d](3ea658d))
* Ensure label only focuses the first labelable child
([#7553](#7553))
([426159c](426159c))
* **flow:** Catch error when beforeBack promise is rejected
([#7601](#7601))
([cb70671](cb70671))
* **input-date-picker, input-time-picker:** Do not show dropdown
affordance when read-only
([#7559](#7559))
([5a3f19c](5a3f19c))
* **input, input-number:** Correctly sanitize numbers when pasting
string with 'e'
([#7648](#7648))
([b8bc11c](b8bc11c))
* **list, sortable-list, value-list:** Emit calciteListOrderChange when
dragging between lists
([#7614](#7614))
([4653581](4653581))
* **list:** Fixes dragging nested list items
([#7555](#7555))
([c25f7b3](c25f7b3))
* **list:** Stop emitting calciteListChange when a list-item is disabled
or closed.
([#7624](#7624))
([7008463](7008463))
* **loader:** Tweak loading animations to work in Safari
([#7564](#7564))
([2103654](2103654))
* **modal:** Catch error when beforeClose promise is rejected
([#7600](#7600))
([70405d0](70405d0))
* **modal:** Handle removal of open attribute and prevent multiple
beforeClose calls
([#7470](#7470))
([f31588f](f31588f))
* **rating:** Adds focus outline on click
([#7341](#7341))
([af30073](af30073))
* **segmented-control:** Refresh items when added dynamically
([#7567](#7567))
([2e36eb3](2e36eb3))
* **split-button:** Update divider and borders to follow design spec
([#7568](#7568))
([8df59ab](8df59ab))
* **tree-item:** Move focus outline to item label area
([#7581](#7581))
([1327cef](1327cef))
* **tree-item:** Updates state when selection changes programmatically
for `selection-mode='ancestors'`
([#7572](#7572))
([40758c5](40758c5))
* **tree:** Improve keyboard navigation
([#7618](#7618))
([826a5cb](826a5cb))
</details>

<details><summary>@esri/calcite-components-react: 1.7.0</summary>

##
[1.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@1.6.1...@esri/calcite-components-react@1.7.0)
(2023-09-01)


### Bug Fixes

* Make sure components are defined in environments like in codesandbox
([#7632](#7632))
([7005cce](7005cce))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^1.7.0-next.22 to ^1.7.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

None yet

4 participants