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.

Sign up for GitHub

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

Jump to bottom

fix(tree): allow deselection in single selectionMode #9363

Merged
merged 20 commits into from
May 23, 2024
Merged

fix(tree): allow deselection in single selectionMode #9363

merged 20 commits into from
May 23, 2024

Conversation

josercarcamo
Copy link
Contributor

Related Issue: #7900

Summary

Made sure an item can be selected and deselected in single selection mode.

@josercarcamo josercarcamo requested a review from a team as a code owner May 17, 2024 22:29
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label May 17, 2024
driskull
driskull approved these changes May 17, 2024
View reviewed changes
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.

Code looks good. For the PR title, maybe:

fix(tree): allow deselection in single selectionMode

or

fix(tree): support unselecting in single selectionMode

jcfranco
jcfranco approved these changes May 18, 2024
View reviewed changes
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.

🎉

packages/calcite-components/src/components/tree/tree.tsx Outdated
@@ -228,7 +228,7 @@ export class Tree {
} else if (!isNoneSelectionMode) {
targetItems.forEach((treeItem) => {
if (!treeItem.disabled) {
treeItem.selected = true;
treeItem.selected = this.selectionMode === "single" ? !treeItem.selected : true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: this.selectionMode === "single" || !treeItem.selected;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the two lines are not logically equivalent: The suggested line will return "true" whenever "selectionMode" is "single"; however, the existing line can return true or false when "selectionMode" is "single". The line can be changed to:

this.selectionMode !== "single" || !treeItem.selected"

Copy link
Member

Choose a reason for hiding this comment

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

Changes look good. 👍 Merge approved

@josercarcamo josercarcamo changed the title fix(tree): allow selection/deselection in single mode fix(tree): allow deselection in single selectionMode May 21, 2024
@josercarcamo josercarcamo added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label May 22, 2024
@josercarcamo josercarcamo merged commit cb6ef73 into main May 23, 2024
13 checks passed
@josercarcamo josercarcamo deleted the josercarcamo/7900-tree-single-select branch May 23, 2024 00:05
@calcite-admin calcite-admin mentioned this pull request May 23, 2024
Merged
benelan added a commit that referenced this pull request May 24, 2024
@benelan
Merge remote-tracking branch 'origin/main' into benelan/7889-clear-ma…
fc01c66 
…x-constraint

* origin/main: (37 commits)
  build(deps): update dependency @stencil/core to v4.18.2 (#9266)
  refactor(stepper-item): drop unnecessary transitions (#9327)
  docs(contributing): add commit message formatting details (#9389)
  docs: update component READMEs (#9328)
  build: update browserslist db (#9263)
  chore: release next
  fix(tree): allow single select only and add indicator (#9405)
  chore: release next
  feat(split-button): Make dividers consistent (#9402)
  chore: release next
  feat(stepper, stepper-item): add separate change events to stepper and items (deprecates `calciteStepperItemChange` on the parent) (#9351)
  chore(tree): fix multiple selection-mode values set in slotted dropdown-groups (#9378)
  fix(dropdown-group): title scale with dropdown scale (#9360)
  chore: release next
  fix(tree): allow deselection in single selectionMode (#9363)
  chore: Update placeholder images in stories and demos (#9395)
  chore: use local placeholder image in html files (#9392)
  chore: release next
  chore: release hotfix (#9382)
  fix(combobox): fix error that occurs when a click is emitted when the component is appended to the DOM (#9380)
  ...
benelan added a commit that referenced this pull request May 29, 2024
@calcite-admin @github-actions @benelan
chore: release main (#9237)
6f7d909 
🤖 I have created a release *beep* *boop*
---


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

##
[2.9.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@2.8.3...@esri/calcite-components@2.9.0)
(2024-05-28)


### Features

* **split-button:** Make dividers consistent
([#9402](#9402))
([caf27e3](caf27e3))
* **stepper, stepper-item:** Add separate change events to stepper and
items (deprecates `calciteStepperItemChange` on the parent)
([#9351](#9351))
([cf6a118](cf6a118))


### Bug Fixes

* **dropdown-group:** Title scale with dropdown scale
([#9360](#9360))
([3529cdd](3529cdd))
* **input-date-picker, date-picker:** Ensure min/max can be unset
([#9406](#9406))
([89b0bfe](89b0bfe))
* **tree:** Allow deselection in single selectionMode
([#9363](#9363))
([cb6ef73](cb6ef73))
* **tree:** Allow single select only and add indicator
([#9405](#9405))
([0d07b59](0d07b59))
</details>

<details><summary>@esri/calcite-components-angular: 2.9.0</summary>

##
[2.9.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-angular@2.8.3...@esri/calcite-components-angular@2.9.0)
(2024-05-28)


### Miscellaneous Chores

* **@esri/calcite-components-angular:** Synchronize components versions


### Dependencies

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

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

##
[2.9.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@2.8.3...@esri/calcite-components-react@2.9.0)
(2024-05-28)


### Miscellaneous Chores

* **@esri/calcite-components-react:** Synchronize components versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^2.9.0-next.23 to ^2.9.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>
Co-authored-by: Ben Elan <no-reply@benelan.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull approved these changes

@jcfranco jcfranco jcfranco approved these changes

Assignees

@josercarcamo josercarcamo

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2024-05-28 - May Main Release
Development

Successfully merging this pull request may close these issues.
3 participants
@josercarcamo @jcfranco @driskull