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(list, sortable-list, value-list): Emit calciteListOrderChange when dragging between lists #7614

Merged
merged 18 commits into from
Aug 31, 2023
Merged

fix(list, sortable-list, value-list): Emit calciteListOrderChange when dragging between lists #7614

merged 18 commits into from
Aug 31, 2023

Conversation

driskull
Copy link
Member

@driskull driskull commented Aug 29, 2023 •
edited
Loading

Related Issue: #7046

Summary

TLDR: Basically, we need to pause draggable component connected/disconnected lifecycle events while a component is being dragged. Having these lifecycle methods kick off during a drag causes SortableJS errors.

  • SortableComponent
    • Refactors logic to prevent any SortableJS component from doing its lifecycle logic when a component is being dragged
    • Previously, it was preventing all SortableJS components Sortable from being destroyed or created but that was causing issues by not emitting events when an item was moved from one list to another.
    • The nested component check that was previously being used isn't ideal because two different lists don't have to be nested to drag items between each other.
    • We need all lists to still continue emitting events when necessary, we just don't want their lifecycle methods to kick off when an item is being dragged. Otherwise, JS errors are thrown.
  • Components
    • Updates SortableComponent components to not do any lifecycle callbacks when an item is being dragged to prevent any JS errors that SortableJS was throwing.
    • This was because in connectedCallback, sortable components were setting up the sortable instance, connecting the observer, modifying items, etc. We don't want the component to do this while an item is being dragged.
    • The same thing as above was happening on disconnectedCallback.
    • This fix stops all those errors that occurred while dragging an item from one list to another.

Assumptions

It is reasonable to not do any lifecycle events for any draggable component while a component is being dragged

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Aug 29, 2023
driskull
driskull commented Aug 29, 2023
View reviewed changes
Copy link
Member Author

@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.

questions

packages/calcite-components/src/utils/sortableComponent.ts Show resolved Hide resolved
packages/calcite-components/src/utils/sortableComponent.ts Show resolved Hide resolved
packages/calcite-components/src/components/list/list.tsx Outdated
@@ -477,6 +486,7 @@ export class List implements InteractiveComponent, LoadableComponent, SortableCo
}

onDragSort(event: SortableEvent): void {
this.parentListEl = this.el.parentElement.closest("calcite-list");
Copy link
Member Author

Choose a reason for hiding this comment

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

Since connectedCallback won't be fired, we should update the parentList here once dragging/sorting is done.

@driskull driskull marked this pull request as ready for review August 31, 2023 20:36
@driskull driskull requested a review from a team as a code owner August 31, 2023 20:36
@driskull driskull requested a review from jcfranco August 31, 2023 20:36
@driskull driskull mentioned this pull request Aug 31, 2023
Open
@driskull
Copy link
Member Author

Test refactor issue: #7643

@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Aug 31, 2023
jcfranco
jcfranco approved these changes Aug 31, 2023
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/list/list.e2e.ts
it("works using a mouse", async () => {
const page = await createSimpleList();

await page.$eval("calcite-list", (list: HTMLCalciteListElement) => {
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 add a note about why this needs to be tested this way vs using an event spy? Applies to the test below.

@driskull driskull removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Aug 31, 2023
@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Aug 31, 2023
@driskull driskull merged commit 4653581 into main Aug 31, 2023
14 checks passed
@driskull driskull deleted the dris0000/sortable-fixes branch August 31, 2023 23:37
@github-actions github-actions bot added this to the 2023 August Priorities milestone Aug 31, 2023
@calcite-admin calcite-admin mentioned this pull request Aug 31, 2023
Merged
benelan pushed a commit that referenced this pull request Sep 1, 2023
@calcite-admin @github-actions
chore: release main (#7571)
784cc25 
🤖 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>
benelan added a commit that referenced this pull request Sep 1, 2023
@benelan
Merge remote-tracking branch 'origin/main' into benelan/move-actions
253292f 
* origin/main: (35 commits)
  ci: make sure to exit on maitenance milestone failure (#7656)
  chore: release next
  fix(block): provide textual name on collapse and expansion to AT (#7652)
  chore: release main (#7571)
  chore: release next
  fix(block, block-section): improve a11y (#7557)
  chore: release next
  fix: add drag styles for improved UX (#7644)
  fix(input, input-number): correctly sanitize numbers when pasting string with 'e' (#7648)
  chore: release next
  feat(flow): add support for custom flow-item elements (#7608)
  chore: release next
  fix(list, sortable-list, value-list): Emit calciteListOrderChange when dragging between lists (#7614)
  feat(input-number): add integer property (#7646)
  chore: release next
  fix(accordion, accordion-item): improve a11y (#7560)
  refactor(stepper, stepper-item): `getElementProp` is refactored out in favor of inheritable props set directly on parent (#7593)
  docs(contributing): update the commit message format example URL (#7641)
  chore: release next
  feat(input-date-picker): normalize year to current century for user typed values only (#7638)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

Assignees

@driskull driskull

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
2023 August Priorities
Development

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