react-reorder
react-reorder copied to clipboard
JakeSidSmith
V3 Alpha Issues
If you've been testing out the V3 alpha, please post your issues here.
It'd also be good to know if V3 has fixed any existing issues you've had.
Reported issues
- [x] Remove engines from package.json (stupid yarn)
- [x]
Unknown reorderIderror when unmounting a group - [ ] Dragging to a space between items in another group should still place the item in this group
- [x] Auto-scroll toGroup when dragging to another group (not fromGroup)
- [x] Do not drag to new group if outside visible area (check root node collision)
- [x] Serious performance issues with 30+ reorder components
- [ ] Inputs do not retain DOM state when dragging between lists
- [ ] Gatsby
window is not defined(move listeners tocomponentDidMount) - [ ] Avoid use of deprecated methods
Requested features
- [ ] Option to start reorder only after movement
- [ ] Handle / draggable area
Seems to work great - just one annoyance, the click/drag behavior doesn't seem very natural when you use a hold time, and if you use a short hold time, it intercepts clicks on internal targets.
For it to feel natural, it should only start reordering once you actually touch/click + drag a few pixels. Then the hold time doesn't matter…
In any case, this was super easy to implement 👍
I wonder if it would be better to get the onClick from React, and the rest from document, so that the normal event bubbling works in React?
Also, would it be possible to have a drag handle instead of the whole element?
For supporting a handle, perhaps allow specifying a handle classname and only if the click target has the handle class, proceed with the reorder?
BTW, I did have one problem: Yarn doesn't like to install the npm package because the Node version is capped at v6. I don't see that line in package.json on the rework branch, so could you push a new version?
You can't install the alpha, or do you mean to push a new major version?
@JakeSidSmith If I run yarn add react-reorder, it will complain that my Node is too new to work with the package, because of the version limit on engine. I think there shouldn't be a version limit at all, seeing as how this is meant for the browser?
As you can see in https://unpkg.com/[email protected]/package.json, the package has
"engines": {
"node": "6.9.1",
"npm": "3.10.8"
}
@wmertens didn't realise that was in the alpha package.json.
I will remove.
[Insert giant rant about yarn here]
In v3 children are no longer wrapped in a div which has the event handlers applied to it, but instead have the event handlers applied to the child directly. This results in children that are custom components being passed the event handlers as props that have to manually be applied to the resulting DOM element.
Now: (child Item component isn't wrapped)
Before: (child Task component was wrapped in a div)
(I realise they're images of different examples :( )
Am I meant to manually pass down style, className, onMouseDown, onTouchDown, and data-dragged to the resulting DOM element or am I doing something wrong to not be getting the old "wrapped" element behaviour?
When a Reorder component with a reorderGroup prop is unmounted an Unknown reorderId error is thrown.
This block of code is where the error is thrown as a result of registering a component with a reorderGroup not adding the id to the reorderComponents object.
@niftykins thanks for your feedback.
The new version works by cloning any children of the Reorder component, and adds the listeners it needs to handle the reordering at this point. There is no wrapper. Is the problem that your events are not being fired?
As for the reorderId thing, I'll try to take a look when I have a chance, that definitely sounds like a bug.
(Note: code is typed out from memory so I could be forgetting to include some things)
If I change a set of children from
this.state.list.map(function (item) {
return (
<li
key={item.name}
className={[classNames.listItem, classNames.listItem2].join(' ')}
style={{color: item.color}}
>
{item.name}
</li>
);
}.bind(this)).toArray()
to
this.state.list.map(function (item) {
return <Item key={item.name} item={item} />;
}.bind(this)).toArray()
I have to manually apply all the props that react-reorder adds in order to achieve the same functionality, thus the resulting component becomes:
function Item(props) {
return (
<li
onMouseDown={props.onMouseDown}
onTouchStart={props.onTouchStart}
className={[classNames.listItem, classNames.listItem2, props.className].join(' ')}
style={Object.assign({}, props.style, {color: props.item.color})}
data-placeholder={props['data-placeholder']}
data-dragged={props['data-dragged']}
>
{props.item.name}
</li>
);
}
Having to manually apply the props isn't much of an issue. It just took me awhile to figure out why my reorderable items weren't able to be "picked up" anymore after factoring them out and turning them into stateless components - so there could probably be some mention of the requirement in the README.
Oh, that is hella weird. At the point that Reorder adds the props, the element should have already been resolved. https://facebook.github.io/react/docs/react-api.html#cloneelement
Will do some investigation at some point. @niftykins, if you want to tweak the examples to demonstrate this, as you did for the unmount issue, that'd be really helpful. :)
@JakeSidSmith https://github.com/niftykins/react-reorder/commit/a406b5ba408a05068381e80830610ce4eba31e03 has a modified example for Lock Vertical
As for the last two blockers for v3, aren't those new features that could be implemented after v3 is released?
Yeah, will probably release after the unmounting thing is sorted. Though I also want to take a look into the above mentioned thing about manually passing props down. There's probably a nicer way to handle this.
Also, would you be interested in a conversion to an ES6 class? It doesn't seem like too much work, and you're using Babel anyway…
On Sun, Aug 6, 2017 at 12:22 PM Jake [email protected] wrote:
Yeah, will probably release after the unmounting thing is sorted. Though I also want to take a look into the above mentioned thing about manually passing props down. There's probably a nicer way to handle this.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JakeSidSmith/react-reorder/issues/78#issuecomment-320498242, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlvdyuaBZlou-pc8kQWezQ6Scamqbks5sVZP7gaJpZM4MfVMv .
@wmertens I had considered this, but having already written v3 in es5, it's quite a bit of work, mainly due to a lack of auto-binding in ES6. Possibly will do this in a future v3.
I meant, I can do it if you like :)
On Sun, Aug 6, 2017, 12:53 PM Jake [email protected] wrote:
@wmertens https://github.com/wmertens I had considered this, but having already written v3 in es5, it's quite a bit of work, mainly due to a lack of auto-binding in ES6. Possibly will do this in a future v3.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JakeSidSmith/react-reorder/issues/78#issuecomment-320499568, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWli9qSKNers2ga6b7dQL5q6p1CFu5ks5sVZsjgaJpZM4MfVMv .
Id' wait until after v3 is released as it's pretty stable at the moment, and will require some retesting.
@wmertens you can start work on that if you like, but things may change a bit while you're doing it.
I imagine there'll be a lot of conflicts.
Just released 3.0.0-alpha.2 with fix for mounting / unmounting reorder groups.
@niftykins been playing around with the issue of passing props to the items in the list. Seems the 2 easiest things to do are:
Wrap your component in a regular element
{
items.map(item => (
<div className="class" key={item.key}>
<Item customProp={thing} />
</div>
))
}
Call a stateless functional component with your props
const Item = ({key, thing}) => (
<li key={key} customProp={thing}>
Content
</li>
);
{
items.map(item => Item({key: item.key, thing}))
}
Perhaps not ideal, but the way reorder works internally now has many more benefits.
Update, probably even easier to just have this if it'd work for your use case:
const Item = (thing, item, index) => (
<li key={item.key} customProp={thing}>
Content
</li>
);
{
items.map(Item.bind(null, thing))
}
Yeah I'm fine with the way it works now, it was just a bit of a surprise because it changed how it worked and I wasn't sure if it was intended or not.
The following approach is what I've been using and has been working alright for me (have to take care to merge values when using ones react-reorder uses too, which is easy enough).
function Item({item, ...otherProps}) {
return (
<div {...otherProps}>
{item.name}
</div>
);
}
I've found another issue :(
Took the time to the alpha version in the project I'm working on over the weekend because a feature I wanted to implement required the kanban style drag and drop. What I discovered is that when there's 30+ (my use-case was a calendar view) reorder components dragging elements around the screen is noticeably lagged.
Upon digging into this I discovered that each reorder component adds a mousemove event, which down the line (triggerGroup) iterates over all of the components in that group and each of them call setState. For a group of 30 components that equals 900 setState calls every time the mouse moves.
Throttling the calls to the move event and triggerGroup didn't seem to fix the issue and I eventually just made it skip the iteration over the group when triggerGroup was called via the mousemove flow which seemed to make the issue go away.
You can view the changes here: https://github.com/niftykins/react-reorder/commit/e8eab167bd620c25513f583050b0569ec91abda1
This seemed more of a hack fix just to get it working, I'm sure with a better understanding of how everything works together you'll be able to come up with something better.
To pile onto @niftykins discovery, I'm finding some performance issue while dragging in IE11. Now, I don't expect IE to be as fast as say chrome, but I've only got 12 items to drag around on the screen, and the translation from dragging is coming off as really laggy. I might have to find a different library if it persists, which I'd really rather not do, since yours works the best.
If you could dig into what niftykins said about the triggerGroup, that would be very helpful.
@ndugger I've since been doing some digging into general performance improvements in React applications. Can you tell me:
Are your reorderable items or any of their props created in the render method, or mapStateToProps?
Are you using fat arrows or bind in your JSX?
A code snippet would be very useful. :)
I'll try to get a MVCE set up for you; in the meantime, you should experiment with using CSS translation instead of using a fixed position with top and left. I think translation would be much more performant.