Skip to content
This repository has been archived by the owner on Jun 5, 2020. It is now read-only.

Reorder actions #64

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Reorder actions #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d016e45
Reorder actions
zalmoxisus Jan 4, 2017
12a36ff
More reliable className checking for dragging
zalmoxisus Jan 28, 2017
cbd5034
Add benchmark
zalmoxisus Feb 17, 2017
164f72c
Benchmark updates (with maxAge) not only inserts
zalmoxisus Feb 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions benchmark/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
const Benchmark = require('benchmark');
require('./jsdom');
const createElement = require('react').createElement;
const render = require('react-test-renderer').create;
const Component = require('../lib').default;

const state = {
actionsById: {},
computedStates: [],
currentStateIndex: -1,
nextActionId: 0,
skippedActionIds: [],
stagedActionIds: [],
monitorState: {
selectedActionId: null,
startActionId: null,
inspectedActionPath: [],
inspectedStatePath: [],
tabName: 'Diff'
}
};

function addNewAction() {
const nextId = state.nextActionId;
state.stagedActionIds = state.stagedActionIds.concat(nextId);
state.actionsById = Object.assign(state.actionsById,
{ [nextId]: { action: { type: 'ACTION' }, timestamp: 1 } }
);
state.computedStates = state.computedStates.concat({ state: {} });
state.currentStateIndex++;
state.nextActionId++;
}

function removeFirstAction() {
delete state.actionsById[state.stagedActionIds[0]];
state.stagedActionIds = state.stagedActionIds.slice(1);
}

const instance = render(createElement(Component, state));

const suite = new Benchmark.Suite;
suite.add('Insert', function() {
addNewAction();
instance.update(createElement(Component, state));
})
.add('Update', function() {
removeFirstAction()
addNewAction();
instance.update(createElement(Component, state));
})
.on('cycle', function(event) {
console.log(String(event.target));
})
.run({ 'async': true });
13 changes: 13 additions & 0 deletions benchmark/jsdom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const jsdom = require('jsdom').jsdom;

global.document = jsdom('');
global.window = document.defaultView;
global.navigator = { userAgent: 'node.js' };

const exposedProperties = ['document', 'window', 'navigator'];
Object.keys(window).forEach((property) => {
if (typeof global[property] === 'undefined') {
exposedProperties.push(property);
global[property] = window[property];
}
});
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"babel-preset-react": "^6.3.13",
"babel-preset-stage-0": "^6.3.13",
"base16": "^1.0.0",
"benchmark": "^2.1.3",
"chokidar": "^1.6.1",
"clean-webpack-plugin": "^0.1.8",
"eslint": "^3.9.1",
Expand All @@ -43,6 +44,7 @@
"export-files-webpack-plugin": "0.0.1",
"html-webpack-plugin": "^2.8.1",
"imports-loader": "^0.6.5",
"jsdom": "^9.11.0",
"json-loader": "^0.5.4",
"nyan-progress-webpack-plugin": "^1.1.4",
"pre-commit": "^1.1.3",
Expand All @@ -55,6 +57,7 @@
"react-redux": "^4.4.0",
"react-router": "^3.0.0",
"react-router-redux": "^4.0.2",
"react-test-renderer": "^15.4.2",
"react-transform-hmr": "^1.0.2",
"redux": "^3.3.1",
"redux-devtools": "^3.1.0",
Expand All @@ -81,6 +84,7 @@
"jss-vendor-prefixer": "^3.0.1",
"lodash.debounce": "^4.0.3",
"react-base16-styling": "^0.4.1",
"react-dragula": "^1.1.17",
"react-json-tree": "^0.10.0",
"react-pure-render": "^1.0.2",
"redux-devtools-themes": "^1.0.0"
Expand Down
33 changes: 32 additions & 1 deletion src/ActionList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { Component } from 'react';
import ReactDOM from 'react-dom';
import dragula from 'react-dragula';
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@zalmoxisus zalmoxisus Jan 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@zalmoxisus zalmoxisus Jan 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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>

Copy link
Collaborator Author

@zalmoxisus zalmoxisus Jan 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@zalmoxisus zalmoxisus Feb 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@zalmoxisus zalmoxisus Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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)

import ActionListRow from './ActionListRow';
import ActionListHeader from './ActionListHeader';
import shouldPureComponentUpdate from 'react-pure-render/function';
Expand All @@ -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);
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

ok

this.drake = dragula([container], {
copy: false,
copySortSource: false,
mirrorContainer: container,
accepts: (el, target, source, sibling) => (
!sibling || parseInt(sibling.getAttribute('data-id'))
),
moves: (el, source, handle) => (
parseInt(el.getAttribute('data-id')) &&
!/\bselectorButton\b/.test(handle.className)
),
}).on('drop', (el, target, source, sibling) => {
let beforeActionId = Infinity;
if (sibling && sibling.className.indexOf('gu-mirror') === -1) {
beforeActionId = parseInt(sibling.getAttribute('data-id'));
}
const actionId = parseInt(el.getAttribute('data-id'));
this.props.onReorderAction(actionId, beforeActionId)
});
}

componentWillUnmount() {
if (this.drake) this.drake.destroy();
}

componentDidUpdate(prevProps) {
Expand All @@ -29,6 +55,8 @@ export default class ActionList extends Component {

scrollToBottom(force) {
const el = ReactDOM.findDOMNode(this.refs.rows);
if (!el) return;

const scrollHeight = el.scrollHeight;
if (force || Math.abs(scrollHeight - (el.scrollTop + el.offsetHeight)) < 50) {
el.scrollTop = scrollHeight;
Expand Down Expand Up @@ -57,13 +85,16 @@ export default class ActionList extends Component {
{filteredActionIds.map(actionId =>
<ActionListRow key={actionId}
styling={styling}
actionId={actionId}
isInitAction={!actionId}
isSelected={
startActionId !== null &&
actionId >= startActionId && actionId <= selectedActionId ||
actionId === selectedActionId
}
isInFuture={actionId > currentActionId}
isInFuture={
actionIds.indexOf(actionId) > actionIds.indexOf(currentActionId)
}
onSelect={(e) => onSelect(e, actionId)}
timestamps={getTimestamps(actions, actionIds, actionId)}
action={actions[actionId].action}
Expand Down
14 changes: 8 additions & 6 deletions src/ActionListRow.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export default class ActionListRow extends Component {
shouldComponentUpdate = shouldPureComponentUpdate

render() {
const { styling, isSelected, action, isInitAction, onSelect,
const { styling, isSelected, action, actionId, isInitAction, onSelect,
timestamps, isSkipped, isInFuture } = this.props;
const { hover } = this.state;
const timeDelta = timestamps.current - timestamps.previous;
Expand All @@ -41,6 +41,8 @@ export default class ActionListRow extends Component {
onMouseEnter={this.handleMouseEnter}
onMouseLeave={this.handleMouseLeave}
onMouseDown={this.handleMouseDown}
onMouseUp={this.handleMouseEnter}
data-id={actionId}
{...styling([
'actionListItem',
isSelected && 'actionListItemSelected',
Expand Down Expand Up @@ -91,22 +93,22 @@ export default class ActionListRow extends Component {

handleMouseEnter = e => {
if (this.hover) return;
this.handleMouseLeave.cancel();
this.handleMouseEnterDebounced(e.buttons);
}

handleMouseEnterDebounced = debounce((buttons) => {
if (buttons) return;
this.setState({ hover: true });
}, 300)
}, 150)

handleMouseLeave = () => {
handleMouseLeave = debounce(() => {
this.handleMouseEnterDebounced.cancel();
if (this.state.hover) this.setState({ hover: false });
}
}, 100)

handleMouseDown = e => {
if (e.target.className.indexOf('selectorButton') === 0) return;
if (this.handleMouseEnterDebounced) this.handleMouseEnterDebounced.cancel();
if (this.state.hover) this.setState({ hover: false });
this.handleMouseLeave();
}
}
24 changes: 17 additions & 7 deletions src/DevtoolsInspector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { getBase16Theme } from 'react-base16-styling';
import { reducer, updateMonitorState } from './redux';
import { ActionCreators } from 'redux-devtools';

const { commit, sweep, toggleAction, jumpToAction, jumpToState } = ActionCreators;
const { commit, sweep, toggleAction, jumpToAction, jumpToState, reorderAction } = ActionCreators;

function getLastActionId(props) {
return props.stagedActionIds[props.stagedActionIds.length - 1];
Expand Down Expand Up @@ -86,6 +86,7 @@ export default class DevtoolsInspector extends Component {
initialScrollTop: PropTypes.number
}),
preserveScrollTop: PropTypes.bool,
draggableActions: PropTypes.bool,
stagedActions: PropTypes.array,
select: PropTypes.func.isRequired,
theme: PropTypes.oneOfType([
Expand All @@ -100,6 +101,7 @@ export default class DevtoolsInspector extends Component {
static defaultProps = {
select: (state) => state,
supportImmutable: false,
draggableActions: true,
theme: 'inspector',
invertTheme: true
};
Expand All @@ -120,7 +122,9 @@ export default class DevtoolsInspector extends Component {
}

updateSizeMode() {
const isWideLayout = this.refs.inspector.offsetWidth > 500;
const node = this.refs.inspector;
if (!node) return;
const isWideLayout = node.offsetWidth > 500;

if (isWideLayout !== this.state.isWideLayout) {
this.setState({ isWideLayout });
Expand All @@ -136,7 +140,9 @@ export default class DevtoolsInspector extends Component {
getCurrentActionId(nextProps, nextMonitorState) ||
monitorState.startActionId !== nextMonitorState.startActionId ||
monitorState.inspectedStatePath !== nextMonitorState.inspectedStatePath ||
monitorState.inspectedActionPath !== nextMonitorState.inspectedActionPath
monitorState.inspectedActionPath !== nextMonitorState.inspectedActionPath ||
this.props.computedStates !== nextProps.computedStates ||
this.props.stagedActionIds !== nextProps.stagedActionIds
) {
this.setState(createIntermediateState(nextProps, nextMonitorState));
}
Expand All @@ -148,7 +154,7 @@ export default class DevtoolsInspector extends Component {
}

render() {
const { stagedActionIds: actionIds, actionsById: actions, computedStates,
const { stagedActionIds: actionIds, actionsById: actions, computedStates, draggableActions,
tabs, invertTheme, skippedActionIds, currentStateIndex, monitorState } = this.props;
const { selectedActionId, startActionId, searchValue, tabName } = monitorState;
const inspectedPathType = tabName === 'Action' ? 'inspectedActionPath' : 'inspectedStatePath';
Expand All @@ -162,16 +168,16 @@ export default class DevtoolsInspector extends Component {
ref='inspector'
{...styling(['inspector', isWideLayout && 'inspectorWide'], isWideLayout)}>
<ActionList {...{
actions, actionIds, isWideLayout, searchValue, selectedActionId, startActionId
actions, actionIds, isWideLayout, searchValue, selectedActionId, startActionId,
skippedActionIds, draggableActions, styling
}}
styling={styling}
onSearch={this.handleSearch}
onSelect={this.handleSelectAction}
onToggleAction={this.handleToggleAction}
onJumpToState={this.handleJumpToState}
onCommit={this.handleCommit}
onSweep={this.handleSweep}
skippedActionIds={skippedActionIds}
onReorderAction={this.handleReorderAction}
currentActionId={actionIds[currentStateIndex]}
lastActionId={getLastActionId(this.props)} />
<ActionPreview {...{
Expand Down Expand Up @@ -199,6 +205,10 @@ export default class DevtoolsInspector extends Component {
}
};

handleReorderAction = (actionId, beforeActionId) => {
if (reorderAction) this.props.dispatch(reorderAction(actionId, beforeActionId));
};

handleCommit = () => {
this.props.dispatch(commit());
};
Expand Down
19 changes: 18 additions & 1 deletion src/utils/createStylingFromTheme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,24 @@ const getSheetFromColorMap = map => ({
},

actionListRows: {
overflow: 'auto'
overflow: 'auto',

'& div.gu-transit': {
opacity: '0.3'
},

'& div.gu-mirror': {
position: 'fixed',
opacity: '0.8',
height: 'auto !important',
'border-width': '1px',
'border-style': 'solid',
'border-color': map.LIST_BORDER_COLOR
},

'& div.gu-hide': {
display: 'none'
}
},

actionListHeaderSelector: {
Expand Down