Reorder actions #64

zalmoxisus · 2017-01-04T15:20:52Z

Implementation of zalmoxisus/redux-devtools-instrument#13.

Here's how it works:

Or a more practical example:

alexkuz · 2017-01-04T22:06:09Z

Looks great! I'm going to review it (and rest of stuff) this weekend. Consider it my new year's resolution :)

zalmoxisus · 2017-01-06T10:41:15Z

Just make sure to update redux-devtools-instrument, otherwise the sorting will be just visually. We could use it directly instead of redux-devtools to import ActionCreators.

alexkuz · 2017-01-08T18:02:52Z

src/ActionList.jsx

@@ -1,5 +1,6 @@
 import React, { Component } from 'react';
 import ReactDOM from 'react-dom';
+import dragula from 'react-dragula';


I have mixed feelings about this lib. It's simple and it works, which is great, but it's not so react-friendly (and it uses css hardcoded classes). I would suggest at least move this with all the dirty stuff in a separate component, if it's possible.

I tried react-sortable-hoc first, but it requires too much changes here, and there were some issues. Didn't notice any issues with react-dragula. It allows us easy to switch draggable feature on/off and also we will be able to duplicate actions in future.

Those classes are added only while dragging. We could just set draggableActions prop to false by default, so it will have no impact when not enabled.

However, if you think this feature is not needed here, we could just add a prop to pass a custom ActionList component. It would also allow for better customizations. For example, I was planning to add props to hide buttons since we'll not need then in the extension.

We could just use a fork for the extension if you think the purposes are different and you don't want to add extra complexity here. For example, including react-split-pane per #62, will also use global classes, and even worse prefixed. It is not a problem for the extension, but could be for Redux DevTools. Let me know what you think would be the best.

I don't mind having this feature, but it kinda forces us to use native DOM (including data attributes) and it concerns me.

Have you tried react-dnd, BTW? I know, it looks a bit scary at first, but it's pretty good, Dan did a great job there :)
http://gaearon.github.io/react-dnd/examples-sortable-simple.html

Yes, I was using it here, but those decorators add complexity as well as react-sortable-hoc. We need 2 different ActionList components - one draggable (with the decorator) and another one not (when having draggableActions propfalse). Also we need to update local state to drop the element before getting the data from the client part (which takes time in case of the extension). I see your point against using the DOM here, though it does its job. Not sure I'll have time in the next days for it (as other issues and 3.0 are waiting). Maybe someone else could continue the work here.

We need 2 different ActionList components - one draggable (with the decorator) and another one not (when having draggableActions prop false)

Or you can just use DragSource.canDrag :)

It's ok to use react-dragula for now, it just would be nice to have it separated, so it could be easily replaced later.

Well, ActionList is a bit more than just a plain list of rows, so I would introduce <DraggableList /> instead of <div {...styling('actionListRows')} ref='rows'> that would be responsible just for dragging, so it would be like:

<DraggableList onReorderAction={...} ...> {ids.map(id => <DraggableListRow ... ><ActionListRow ... /></DraggableListRow>} </DraggableList>

I see. In this case I'd pick react-sortable-hoc, which simplifies it and also have some killer features like locking dragging inside the parent container or axis.

However, the reason I picked Dragula is not to implement that. Action list items are the components which affect the performance the most. So, due to maxAge, we can have lots of such components removed and added per second.

Taking into account that this feature would be used occasionally by less than 5% of users, we either find another way not to introduce any complexity here or not implement it at all. A solution would be to have a button to enable dragging (so we'll rerender ActionListRows with added DraggableListRow), but, since Dragula offers it without any costs, I don't think it is worth to add changes to the UI.

Maybe I missed something, feel free to amend my pr.

Yeah, react-sortable-hoc looks much better. Actually, it would be interesting to measure it - is there really noticeable difference in performance for high-loaded actions.
By the way, it might be useful to utilize something like react-virtualized for this list.

I was thinking about react-virtualized too, but it wouldn't help much, at least for the extension where maxAge is 50 by default, and, anyway, we shouldn't hide (virtualize) the new added action rows. The perf waste is due to creating new elements for every dispatched action. This is the part where React is very slow, and we should also take into consideration that unlike the extension, for vanilla Redux DevTools, React is in dev mode, making it even slower.

Anyway, Inspector is twice faster than LogMonitor. The fastest is Chart monitor, despite of lots of d3 animations there.

I added recently autoincrement button to the demo and, as expected, it was crashing the extension. Surprisingly, it was mostly due to React, not to serialization. Just bouncing the data sent to the extension solved the problem.

A benchmark would be rather useful indeed, so we could do it for every new feature to see the cost it adds.

Added a benchmark. It starts from an empty action list and adds actions one by one.

Before: 29.16 ops/sec ±25.13% (18 runs sampled).
After adding react-dnd's DragDropContext(HTML5Backend) and DragSource hocs: 25.05 ops/sec ±25.87% (18 runs sampled).

When starting not from an empty list, but having already 500 actions, it's 4.98 ops/sec ±4.29% (without hocs) and 3.64 ops/sec ±4.39% (with react-dnd). But we're usually avoiding such bad perf with maxAge. Also note that I'm benchmarking production build, since it's not recommended to benchmark React in development and we're not using it for the extension. Without the extension we're running on dev environment and the perf should be worse.

Obviously, Dragula has no impact, as nothing is done on component update. Unfortunately, I wasn't able to get react-sortable-hoc working on jsdom (it fails after calling findDOMNode inside the lib), but it shouldn't be much different from react-dnd, as there's lot of work inside the hocs on every update.

If the perf waste would be for a core feature, it could be negotiable, but not for this one. It's not something we'd use frequently, but nice to have (I just got inspired by Dan's workflow). We would also add the ability to duplicate actions (when dragging an action holding Alt key), which should be more popular.

Feel free to make changes in the pr when you have some time. It's not in a hurry, we're using a fork meanwhile.

UPDATE: For the sake of the experiment, I added also a benchmark for updates when using maxAge (so a row is added and the first one is removed).

Without hocs:

Insert x 31.20 ops/sec ±27.06% (17 runs sampled)

Update x 15.74 ops/sec ±1.61% (42 runs sampled)

With hocs:

Insert x 26.61 ops/sec ±27.72% (16 runs sampled)

Update x 13.64 ops/sec ±2.44% (37 runs sampled)

alexkuz · 2017-01-08T18:05:57Z

src/ActionList.jsx

@@ -19,6 +20,31 @@ export default class ActionList extends Component {

  componentDidMount() {
    this.scrollToBottom(true);
+    if (!this.props.draggableActions) return;
+    const container = ReactDOM.findDOMNode(this.refs.rows);


I think it's better to use callback ref instead of string (see https://github.com/bevacqua/react-dragula#example-using-refs-es2015-syntax), as string refs are not recommended recently.

Sure, but it was introduced before and used in other places. Better let's keep it for another PR.

alexkuz · 2017-01-08T18:16:04Z

src/ActionList.jsx

+      ),
+      moves: (el, source, handle) => (
+        parseInt(el.getAttribute('data-id')) &&
+        handle.className.indexOf('selectorButton') !== 0


Did you mean !== -1?
Also, using className here looks unreliable. Can we use data attribute or something? (which is also not great, but better)

We're returning false here, not to drag through the buttons. It could be === -1, but it doesn't guarantee that it's a substring of another class. On the other hand, it implies that this class will be always the first.

Changed to a better checking here.

Reorder actions

d016e45

zalmoxisus force-pushed the reorder-actions branch from 2d55beb to d016e45 Compare January 4, 2017 16:05

alexkuz suggested changes Jan 8, 2017

View reviewed changes

zalmoxisus added 3 commits January 28, 2017 16:43

More reliable className checking for dragging

12a36ff

Add benchmark

cbd5034

Benchmark updates (with maxAge) not only inserts

164f72c

zalmoxisus mentioned this pull request Dec 23, 2018

Extract LiftedState directly from the extension zalmoxisus/redux-devtools-extension#618

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder actions #64

Reorder actions #64

zalmoxisus commented Jan 4, 2017

alexkuz commented Jan 4, 2017

zalmoxisus commented Jan 6, 2017 •

edited

alexkuz Jan 8, 2017

zalmoxisus Jan 8, 2017 •

edited

alexkuz Jan 8, 2017

zalmoxisus Jan 8, 2017 •

edited

alexkuz Jan 9, 2017

alexkuz Jan 14, 2017

zalmoxisus Jan 28, 2017 •

edited

alexkuz Jan 31, 2017

zalmoxisus Feb 1, 2017 •

edited

zalmoxisus Feb 17, 2017 •

edited

alexkuz Jan 8, 2017

zalmoxisus Jan 8, 2017

alexkuz Jan 8, 2017

alexkuz Jan 8, 2017

zalmoxisus Jan 8, 2017

zalmoxisus Jan 28, 2017

Reorder actions #64

Are you sure you want to change the base?

Reorder actions #64

Conversation

zalmoxisus commented Jan 4, 2017

alexkuz commented Jan 4, 2017

zalmoxisus commented Jan 6, 2017 • edited

Choose a reason for hiding this comment

zalmoxisus Jan 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zalmoxisus Jan 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zalmoxisus Jan 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zalmoxisus Feb 1, 2017 • edited

Choose a reason for hiding this comment

zalmoxisus Feb 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zalmoxisus commented Jan 6, 2017 •

edited

zalmoxisus Jan 8, 2017 •

edited

zalmoxisus Jan 8, 2017 •

edited

zalmoxisus Jan 28, 2017 •

edited

zalmoxisus Feb 1, 2017 •

edited

zalmoxisus Feb 17, 2017 •

edited