Issue#271 #956

yuki-katayama · 2024-05-12T05:34:48Z

@lukasoppermann

Description

This issue aims to address two main changes to improve the functionality of the HTML5Sortable library:

Enhancement of Custom Drag Image Selection: Extend the sortable function to allow customDragImage option to accept a string selector, in addition to the existing function type. This enhancement will make it more flexible for users to specify custom drag images directly via CSS selectors.
Testing for Custom Drag Image: Implement tests to ensure that custom drag images defined by CSS selectors are correctly applied, and to handle cases where the selector does not match any elements, ensuring the code robustness.

Tasks

Modify sortable function to handle string selectors for customDragImage.
Update setDragImage function to properly handle NodeList objects derived from string selectors.
Write tests to verify that custom drag images are correctly set when using selectors.
Write tests to ensure proper error handling when selectors do not match any elements.

Related Issues

issue#271

Implementation Video

using code

...
<div class="test-selector bg-teal white">
 test selector
</div>
...
<script>
...
sortable('.js-sortable-oneway-receive', {
 forcePlaceholderSize: true,
 acceptFrom: '.js-sortable-oneway,.js-sortable-oneway-receive',
 placeholderClass: 'mb1 bg-navy border border-yellow',
 customDragImage: '.test-selector'
});
</script>

Test results

-------------------------|----------|----------|----------|----------|-------------------|
File                     |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-------------------------|----------|----------|----------|----------|-------------------|
All files                |    86.37 |    75.23 |       90 |    86.45 |                   |
 attribute.ts            |      100 |      100 |      100 |      100 |                   |
 data.ts                 |      100 |      100 |      100 |      100 |                   |
 debounce.ts             |      100 |      100 |      100 |      100 |                   |
 defaultConfiguration.ts |      100 |      100 |      100 |      100 |                   |
 elementHeight.ts        |      100 |      100 |      100 |      100 |                   |
 elementWidth.ts         |      100 |      100 |      100 |      100 |                   |
 eventListener.ts        |      100 |      100 |      100 |      100 |                   |
 filter.ts               |      100 |      100 |      100 |      100 |                   |
 getEventTarget.ts       |      100 |    66.67 |      100 |      100 |                 6 |
 getHandles.ts           |      100 |       70 |      100 |      100 |             20,25 |
 getIndex.ts             |      100 |      100 |      100 |      100 |                   |
 hoverClass.ts           |      100 |      100 |      100 |      100 |                   |
 html5sortable.ts        |    75.79 |    49.67 |    76.74 |    76.43 |... 23,644,645,659 |
 insertHtmlElements.ts   |      100 |      100 |      100 |      100 |                   |
 isConnected.ts          |      100 |      100 |      100 |      100 |                   |
 isInDom.ts              |      100 |      100 |      100 |      100 |                   |
 makePlaceholder.ts      |      100 |      100 |      100 |      100 |                   |
 offset.ts               |      100 |      100 |      100 |      100 |                   |
 serialize.ts            |      100 |      100 |      100 |      100 |                   |
 setDragImage.ts         |      100 |      100 |      100 |      100 |                   |
 store.ts                |      100 |      100 |      100 |      100 |                   |
 throttle.ts             |      100 |      100 |      100 |      100 |                   |
-------------------------|----------|----------|----------|----------|-------------------|

Test Suites: 25 passed, 25 total
Tests:       7 skipped, 123 passed, 130 total
Snapshots:   0 total
Time:        5.81s, estimated 7s
Ran all test suites.

yuki-katayama · 2024-05-20T11:04:55Z

@lukasoppermann

hi, is this project already finished?

If you find this pull request unnecessary, or if further modifications are required, please feel free to reject or suggest necessary revisions.

lukasoppermann · 2024-05-21T11:21:37Z

src/html5sortable.ts

-      setDragImage(e, dragItem, options.customDragImage)
+      let dragImage: Function | NodeList = null
+      if (typeof options.customDragImage === 'string') {
+        dragImage = document.querySelectorAll(options.customDragImage)


Hey @yuki-katayama do you use querySelectorAll on purpose? Wouldn't it be better to use querySelector in case multiple items with e.g. a class exist?

Hi @lukasoppermann. Thank you for your review.

I think so too about using querySelector instead. At first, I chose querySelectorAll considering the future prospects of the implementation, but I thought it would be a good idea to implement it again when it becomes necessary.

Also, when using querySelector, the code now defaults to the defaultDragImage function instead of throwing an error if the element is not found.

Therefore, we changed the error handling from throwing an exception to logging the error using console.error. Please review the changes and let us know if further adjustments are needed.

Thank you for your feedback!

test results.

PASS __tests__/setDragImage.test.ts PASS __tests__/options.test.ts PASS __tests__/sortableMethodsTests/_removeItemData.test.ts PASS __tests__/serialize.test.ts (5.065s) PASS __tests__/api.test.ts (5.2s) PASS __tests__/isConnected.test.ts (5.241s) PASS __tests__/events/events.test.ts (5.536s) PASS __tests__/elementHeight.test.ts PASS __tests__/sortableMethodsTests/_removeItemEvents.test.ts PASS __tests__/filter.test.ts PASS __tests__/elementWidth.test.ts PASS __tests__/events/hoverClass.test.ts PASS __tests__/store/storeConfig.test.ts PASS __tests__/getHandles.test.ts PASS __tests__/makePlaceholder.test.ts PASS __tests__/insertAfter.test.ts PASS __tests__/throttle.test.ts PASS __tests__/store/storeData.test.ts PASS __tests__/store/storePlaceholder.test.ts PASS __tests__/index.test.ts PASS __tests__/insertBefore.test.ts PASS __tests__/offset.test.ts PASS __tests__/isInDom.test.ts PASS __tests__/debounce.test.ts PASS __tests__/store/store.test.ts -------------------------|----------|----------|----------|----------|-------------------| File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s | -------------------------|----------|----------|----------|----------|-------------------| All files | 86.1 | 74.62 | 89.9 | 86.19 | | attribute.ts | 100 | 100 | 100 | 100 | | data.ts | 100 | 100 | 100 | 100 | | debounce.ts | 100 | 100 | 100 | 100 | | defaultConfiguration.ts | 100 | 100 | 100 | 100 | | elementHeight.ts | 100 | 100 | 100 | 100 | | elementWidth.ts | 100 | 100 | 100 | 100 | | eventListener.ts | 100 | 100 | 100 | 100 | | filter.ts | 100 | 100 | 100 | 100 | | getEventTarget.ts | 100 | 66.67 | 100 | 100 | 6 | getHandles.ts | 100 | 70 | 100 | 100 | 20,25 | getIndex.ts | 100 | 100 | 100 | 100 | | hoverClass.ts | 100 | 100 | 100 | 100 | | html5sortable.ts | 75.55 | 49.03 | 76.74 | 76.19 |... 24,645,646,660 | insertHtmlElements.ts | 100 | 100 | 100 | 100 | | isConnected.ts | 100 | 100 | 100 | 100 | | isInDom.ts | 100 | 100 | 100 | 100 | | makePlaceholder.ts | 100 | 100 | 100 | 100 | | offset.ts | 100 | 100 | 100 | 100 | | serialize.ts | 100 | 100 | 100 | 100 | | setDragImage.ts | 100 | 100 | 100 | 100 | | store.ts | 100 | 100 | 100 | 100 | | throttle.ts | 100 | 100 | 100 | 100 | | -------------------------|----------|----------|----------|----------|-------------------| Test Suites: 25 passed, 25 total Tests: 7 skipped, 123 passed, 130 total Snapshots: 0 total Time: 10.198s Ran all test suites. $ standard 'src/*.ts' '__tests__/*.ts' | snazzy

lukasoppermann · 2024-05-21T11:22:37Z

Sorry for the delay. Looks good @yuki-katayama just one comment.

coveralls · 2024-05-21T11:24:07Z

coverage: 86.188% (-0.3%) from 86.441%
when pulling ae0e0be on yuki-katayama:issue#271
into a8e0596 on lukasoppermann:master.

yuki-katayama added 4 commits May 12, 2024 14:04

Changed so that selector can be selected

28d0810

make tests

6893ea6

fix spell in CONTRIBUTING.md

a45b241

add [Using a Selector] section on README.md

ffc1ffc

lukasoppermann reviewed May 21, 2024

View reviewed changes

change from selectorAll to selector

ae0e0be

lukasoppermann merged commit ea5cab5 into lukasoppermann:master May 28, 2024
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue#271 #956

Issue#271 #956

yuki-katayama commented May 12, 2024 •

edited

yuki-katayama commented May 20, 2024 •

edited

lukasoppermann May 21, 2024

yuki-katayama May 22, 2024 •

edited

yuki-katayama May 22, 2024 •

edited

lukasoppermann commented May 21, 2024

coveralls commented May 21, 2024 •

edited

Issue#271 #956

Issue#271 #956

Conversation

yuki-katayama commented May 12, 2024 • edited

Description

Tasks

Related Issues

Implementation Video

using code

Test results

yuki-katayama commented May 20, 2024 • edited

lukasoppermann May 21, 2024

Choose a reason for hiding this comment

yuki-katayama May 22, 2024 • edited

Choose a reason for hiding this comment

yuki-katayama May 22, 2024 • edited

Choose a reason for hiding this comment

lukasoppermann commented May 21, 2024

coveralls commented May 21, 2024 • edited

yuki-katayama commented May 12, 2024 •

edited

yuki-katayama commented May 20, 2024 •

edited

yuki-katayama May 22, 2024 •

edited

yuki-katayama May 22, 2024 •

edited

coveralls commented May 21, 2024 •

edited