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

fix: set the target as the host element when target contains a shadowRoot #2347

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

driskull
Copy link
Contributor

Fix for sorting elements that contain web components

See #2346 for explanation and code samples of the issue.

When sortable elements contain web components with shadowRoot, and forceFallback is enabled, swapping elements is not working correctly.

DOM Example:

<ul id="list" class="list-group">
  <li class="list-group-item"><custom-handle></custom-handle><custom-content></custom-content></li>
 <li class="list-group-item"><custom-handle></custom-handle><custom-content></custom-content></li>
</li>

Verified

This commit was signed with the committer’s verified signature.
tlambert03 Talley Lambert
…Root. SortableJS#2346
@RotkivDS
Copy link

I love you. Thanks for the fix.

driskull added a commit to Esri/calcite-design-system that referenced this pull request Feb 15, 2024

Verified

This commit was signed with the committer’s verified signature.
tlambert03 Talley Lambert
**Related Issue:** #8728

## Summary

- Fixes dragging when on a mobile device.
- Sortablejs uses fallback code instead of native drag and drop on
mobile devices.
- Patches `sortablejs` dependency until the fix for the issue is
installed and released.
SortableJS/Sortable#2346
- Adds and uses the dependency `patch-package` to handle patching the
dependency.
- Applies the changes listed here:
SortableJS/Sortable#2347

---------

Co-authored-by: JC Franco <jfranco@esri.com>
Elijbet pushed a commit to Esri/calcite-design-system that referenced this pull request Feb 15, 2024
**Related Issue:** #8728

## Summary

- Fixes dragging when on a mobile device.
- Sortablejs uses fallback code instead of native drag and drop on
mobile devices.
- Patches `sortablejs` dependency until the fix for the issue is
installed and released.
SortableJS/Sortable#2346
- Adds and uses the dependency `patch-package` to handle patching the
dependency.
- Applies the changes listed here:
SortableJS/Sortable#2347

---------

Co-authored-by: JC Franco <jfranco@esri.com>
@driskull
Copy link
Contributor Author

@owen-m1 Let me know if there's anything I can do to get this one moving. Thanks

@owen-m1
Copy link
Member

owen-m1 commented Mar 17, 2024

@driskull Thank you for the fix, but unfortunately this code defeats the purpose of the loop for going within shadow DOMs to find the target, since it immediately jumps out of the shadowDOM thus breaking the loop. What would be better is if we move the fix to later on in the function, when we are trying to detect the nearest Sortable to the target, and allow that loop to exit the shadowDOM to traverse parents outside of it.

Specifically your code could be moved to line 788:

while (parent = (parent.parentNode || parent.host));

In fact, we have a util function to make this simpler:

while (parent = getParentOrHost(parent));

Haven't tested this but I believe it should fix the issue and still support targets within a shadowDOM, please let me know if that's not the case

uses getParentOrHost utility and moves logic to appropriate place
@driskull
Copy link
Contributor Author

Thanks @owen-m1, that suggestion works and makes perfect sense. I've updated the PR to reflect the changes.

@owen-m1 owen-m1 merged commit e7b4859 into SortableJS:master Mar 22, 2024
1 of 2 checks passed
driskull added a commit to Esri/calcite-design-system that referenced this pull request Jun 21, 2024
**Related Issue:** #9521

## Summary

- patch sortablejs with the correct update to fix dragging shadow
elements on mobile devices
- SortableJS/Sortable#2347
SortableJS/Sortable#2346
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants