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: Handle element in sortable item should be the most recent #493

Conversation

nicolechung
Copy link

@nicolechung nicolechung commented Oct 5, 2022

Related issue: #492

What changed

  • added a demo to the demos page
  • this demo has a conditionally rendered sort handle
  {{#if this.isEven}}
    <Cells::Sortable @index={{@index}} />
  {{else}}
    <Cells::Sortable @index={{@index}} class="odd-class"/>
  {{/if}}
  • because of this, the {{sort-handle}} is rendered more often than the {{sortable-item}}. Because of this, the this.handleElement in {{sort-item}} is one that is no longer in the document body.
  • adding in a way to get the new this.handleElement in the keyDown handler fixes this issue
  • updated tests

How to test

(refer to the animation in the issue)

  1. start the demo with ember serve
  2. Using your mouse (it's faster) click on the demo I created
  3. Use TAB to select a fruit
  4. Use ENTER to go into drag mode
  5. Use ARROW KEYS to move
  6. Use ENTER/SPACE to commit
  7. Repeat steps 1-6 more than 3 times. It should work every time.

ember test should also pass.

@nicolechung nicolechung force-pushed the handle_element_in_sortable_item_most_recent branch from ac82109 to 565685f Compare October 5, 2022 15:29
- add problem example to the demo
- conditionally rendered sort-handle renders more often than sortable-item
- this creates a this.handleElement that is not attached to the document anymore
- keyboard drag and drop will not work
- check again for the correct handle element when key down is pressed
- this ensures that the this.handleElement is the one that is still in
the document body
@nicolechung nicolechung force-pushed the handle_element_in_sortable_item_most_recent branch from 565685f to a980a7a Compare October 5, 2022 15:35
@nicolechung nicolechung changed the title WIP Handle element in sortable item most recent fix: Handle element in sortable item should be the most recent Oct 5, 2022
- added a test for a conditional rendering of the sort handle
- for this test to fail, drag and drop with keyboard has to be done several times
- added displaying the fruits only as this is easier to read in the
tests
@nicolechung nicolechung force-pushed the handle_element_in_sortable_item_most_recent branch from a980a7a to 96492ee Compare October 5, 2022 19:01
@nicolechung
Copy link
Author

Note: I couldn't seem to run yarn ember try:one ember-lts-3.24 locally:

yarn ember try:one ember-lts-3.24
yarn run v1.22.18
$ /Users/nchung/OpenSource/ember-sortable/node_modules/.bin/ember try:one ember-lts-3.24
Error!
Error: Cannot copy '../../../browserslist/cli.js' to a subdirectory of itself, '../../../browserslist/cli.js'.
Error: Cannot copy '../../../browserslist/cli.js' to a subdirectory of itself, '../../../browserslist/cli.js'.
    at /Users/nchung/OpenSource/ember-sortable/node_modules/ember-try/node_modules/fs-extra/lib/copy/copy.js:210:21
    at FSReqCallback.oncomplete (fs.js:180:23)
error Command failed with exit code 1.

So I fixed the error based on reading the error message.

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

Nice!

@knownasilya knownasilya added the bug label Oct 6, 2022
@@ -268,6 +268,8 @@ export default class SortableItemModifier extends Modifier {
return;
}

this.setupHandleElement();
Copy link
Author

Choose a reason for hiding this comment

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

This is the main change. If you run the demo and comment out this line, you will see the error I reported in the issue.

@NullVoxPopuli NullVoxPopuli merged commit f6508a1 into adopted-ember-addons:master Oct 6, 2022
@NullVoxPopuli
Copy link
Contributor

Released in 4.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants