Keyboard sorting

[b-ryu]

Resolves #77

This PR introduces keyboard-accessible sorting. Users are now able to sort items using only their keyboard.

Devs can set tabIndex on their SortableElements. Once focused:

• Space picks up an item
• Arrow UP/LEFT moves the picked-up item backwards one place in the list
• Arrow DOWN/RIGHT moves the picked-up item forwards one place in the list
• Space again to drop the item
• ESC to cancel a sort

Known issue(s)
On react-infinite, occasionally (every couple hundred items) it'll stop sorting prematurely, due to what I believe is the virtualized DOM not appending items to the list fast enough for the sorting to continue. The fix is simply to stop keyboard sorting, scroll down a bit with the mouse (or tab forward) and continue. Currently looking into possible solutions. The simplest fix that I found was just to increase the preload values passed to Infinite's props, specifically preloadBatchSize and preloadAdditionalHeight.

[clauderic]

Hey @b-ryu,

Thanks for taking the time to submit this PR! Left a few comments / questions / suggestions, I think this is going in the right direction, my main concern is with the way the elements are moved in the DOM currently, I'd prefer using transforms in order to offer the possibility to animate the elements as they shift positions.

On a separate note, I think it'd also be nice from an accessibility perspective to announce what is going on to assist users with visual (or other) impairments:

This doesn't have to happen in this PR, but something worth considering :)

[natew]

Just want to say it's super encouraging to see signs of life with this library. Thanks @clauderic!

I want to submit a PR that just moves to the new Context API (see: https://twitter.com/dan_abramov/status/1097866569701621763). I imagine it's a small PR but was worried it may not be accepted ever. Will try now!

[clauderic]

Hey @b-ryu,

This is looking great, tophatted a bunch of different scenarios and it works surprisingly well. You mentioned there were issues with react-infinite, have you been able to replicate similar issues with the react-virtualized or react-tiny-virtual-list examples?

I've left a few questions / suggestions for now. I'll take a closer look at the logic for moving the items around over the weekend.

As an aside, I'm a bit worried about the amount of complexity and alternative code paths it introduces to the codebase, but that's probably more of a symptom of how the library currently works. In the future, I'd like to refactor the translations to be based on deterministic state updates and only have one code path for translates using getBoundingClientRect to swap items around.

[clauderic]

This is probably a stretch for this PR, so let's keep it for a subsequent PR, but from a UX perspective, arrow up / down in a grid setup should probably move to the previous / next row

[andersahp]

@clauderic are there plans to include this functionality for up/down soon? I mean down arrow means swap element with element below in grid and similar for up arrow. Seems this is not possible at the moment

[clauderic]

This is a nice addition, though sort of unrelated to the current PR as far as I can tell? Unless it's needed specifically for this PR, I'd prefer introducing it in a separate PR

[b-ryu]

First of all, I want to thank you for taking the time to review this PR (again) 😄👍

You mentioned there were issues with react-infinite, have you been able to replicate similar issues with the react-virtualized or react-tiny-virtual-list examples?

Not that I know of, the issue I mentioned seems only to arise when reaching the 300th or so item in a react-infinite list. I seem to be able to shuffle through the entire length of a list with react-virtualized and react-tiny-virtual-list without any issues, but that may take some more scrutiny to confirm.

Update: fixed by using react-infinite's preloading configuration.

As an aside, I'm a bit worried about the amount of complexity and alternative code paths it introduces to the codebase

Good point 👍 I initially considered inserting the logic for some of the key sorting methods into the existing methods since most of the underlying structure was the same, but found that it was a little messy, so I opted to share the ones I could use with minimal modification and extract the rest into their own set of methods.

In the future, I'd like to refactor the translations to be based on deterministic state updates and only have one code path for translates using getBoundingClientRect to swap items around.

Makes sense, caching the initial getBoundingClientRect values would really simplify the positioning logic, for keyUpdatePosition at least 😄

[beefchimi]

Just tested this out locally - awesome work! Really excited about this. I didn't look too closely at the code - my only comments though would have been nitpicky style changes for the examples 🙃 nothing to worry about!

[clauderic]

Not sure why this was needed, commented it out for the time being

[b-ryu]

I was running into issues with correctly positioning the helper and the elements in the cases where there's resizing during sorting.

[clauderic]

We create a mock event here with the coordinates of where the mouse would be if this were a mouse event, so as to be able to re-use the logic for animating nodes. While the copy paste approach worked, it would have made maintaining and contributing to this library exponentially more challenging

[clauderic]

Hey @b-ryu, great work on the refactor, this is looking really close to being ready to merge 👌

Gave this a fairly thorough tophat, it works great. There was only one small bug that I found with keyboard sorting of horizontal lists, it looks like there's an issue with the last index of the list, here's a gif demonstrating the issue:

I believe the same issue exists with mouse sorting actually, so might not be a huge deal for the time being.

[lowewenzel]

I am having trouble implementing this, and also hard to find examples on where tabIndex={0} should be. I am using this with react-virtualized. I tried the tabIndex on the SortableElement as well as the outermost <div> within the SortableElement implementation.

Sorry for bringing up this PR, but docs are pretty minimal on accessibility support. Thanks in advance!

[vaynevayne]

It won't work at all, emmm...

const GridItem = SortableElement(({ value }) => (
  <div style={gridItemStyles} tabIndex={0}>
    {value}
  </div>
))
const Grid = SortableContainer(({ items }) => (
  <div style={gridStyles}>
    {items.map((col, index) => (
      <GridItem key={col.key} index={index} value={col.title} tabIndex={0} />
    ))}
  </div>
))