From 687fb3a201fd0e8a48f95914d0c62d253807bc8e Mon Sep 17 00:00:00 2001 From: Filip Barl Date: Fri, 6 Jan 2017 15:26:05 +0100 Subject: [PATCH 1/3] Maintain focus on hovered node table rows More sophisticated row focusing Keeping deleted focused nodes in the table Fixed focus debouncing --- .../node-details/node-details-table-row.js | 19 +----- .../node-details/node-details-table.js | 67 +++++++++++++++++-- client/app/scripts/constants/timer.js | 1 + .../utils/__tests__/array-utils-test.js | 36 ++++++++++ client/app/scripts/utils/array-utils.js | 11 +++ 5 files changed, 111 insertions(+), 23 deletions(-) diff --git a/client/app/scripts/components/node-details/node-details-table-row.js b/client/app/scripts/components/node-details/node-details-table-row.js index a26bee5518..cba5210c1b 100644 --- a/client/app/scripts/components/node-details/node-details-table-row.js +++ b/client/app/scripts/components/node-details/node-details-table-row.js @@ -81,24 +81,12 @@ export default class NodeDetailsTableRow extends React.Component { this.saveLabelElementRef = this.saveLabelElementRef.bind(this); this.onMouseDown = this.onMouseDown.bind(this); this.onMouseUp = this.onMouseUp.bind(this); - this.onMouseEnter = this.onMouseEnter.bind(this); - this.onMouseLeave = this.onMouseLeave.bind(this); } saveLabelElementRef(ref) { this.labelElement = ref; } - onMouseEnter() { - const { node, onMouseEnterRow } = this.props; - onMouseEnterRow(node); - } - - onMouseLeave() { - const { node, onMouseLeaveRow } = this.props; - onMouseLeaveRow(node); - } - onMouseDown(ev) { const { pageX, pageY } = ev; this.mouseDragOrigin = [pageX, pageY]; @@ -121,8 +109,7 @@ export default class NodeDetailsTableRow extends React.Component { } render() { - const { node, nodeIdKey, topologyId, columns, onClick, onMouseEnterRow, onMouseLeaveRow, - selected, colStyles } = this.props; + const { node, nodeIdKey, topologyId, columns, onClick, selected, colStyles } = this.props; const [firstColumnStyle, ...columnStyles] = colStyles; const values = renderValues(node, columns, columnStyles); const nodeId = node[nodeIdKey]; @@ -132,8 +119,8 @@ export default class NodeDetailsTableRow extends React.Component { h.id === sortedBy); const sortedDesc = this.state.sortedDesc || defaultSortDesc(sortedByHeader); let nodes = getSortedNodes(this.props.nodes, sortedByHeader, sortedDesc); + if (focusedRowIndex && focusedRowIndex < nodes.length) { + const nodeRowIndex = findIndex(nodes, node => node.id === focusedNode.id); + if (nodeRowIndex >= 0) { + // If the focused node still exists in the table, we move it + // to the hovered row, keeping the rest of the table sorted. + moveElement(nodes, nodeRowIndex, focusedRowIndex); + } else { + // Otherwise we insert the dead focused node there, pretending + // it's still alive. That enables the users to read off all the + // info they want and perhaps even open the details panel. Also, + // only if we do this, we can guarantee that mouse hover will + // always freeze the table row until we focus out. + insertElement(nodes, focusedRowIndex, focusedNode); + } + } + const limited = nodes && this.state.limit > 0 && nodes.length > this.state.limit; const expanded = this.state.limit === 0; const notShown = nodes.length - this.state.limit; @@ -178,7 +231,7 @@ export default class NodeDetailsTable extends React.Component { style={this.props.tbodyStyle} onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave}> - {nodes && nodes.map(node => ( + {nodes && nodes.map((node, index) => ( { this.onMouseEnterRow(index, node); }} + onMouseLeave={this.onMouseLeaveRow} topologyId={topologyId} /> ))} diff --git a/client/app/scripts/constants/timer.js b/client/app/scripts/constants/timer.js index 8c8d4dc4e0..fb6eb92f4e 100644 --- a/client/app/scripts/constants/timer.js +++ b/client/app/scripts/constants/timer.js @@ -1,3 +1,4 @@ /* Intervals in ms */ export const API_INTERVAL = 30000; export const TOPOLOGY_INTERVAL = 5000; +export const TABLE_ROW_FOCUS_DEBOUNCE_INTERVAL = 200; diff --git a/client/app/scripts/utils/__tests__/array-utils-test.js b/client/app/scripts/utils/__tests__/array-utils-test.js index 49f0eb4bdf..c24dcba00a 100644 --- a/client/app/scripts/utils/__tests__/array-utils-test.js +++ b/client/app/scripts/utils/__tests__/array-utils-test.js @@ -45,4 +45,40 @@ describe('ArrayUtils', () => { } }); }); + + describe('insertElement', () => { + const f = (array, index, element) => { + ArrayUtils.insertElement(array, index, element); + return array; + }; + + it('it should insert an element into the array at the specified index', () => { + expect(f(['x', 'y', 'z'], 0, 'a')).toEqual(['a', 'x', 'y', 'z']); + expect(f(['x', 'y', 'z'], 1, 'a')).toEqual(['x', 'a', 'y', 'z']); + expect(f(['x', 'y', 'z'], 2, 'a')).toEqual(['x', 'y', 'a', 'z']); + expect(f(['x', 'y', 'z'], 3, 'a')).toEqual(['x', 'y', 'z', 'a']); + }); + }); + + describe('moveElement', () => { + const f = (array, from, to) => { + ArrayUtils.moveElement(array, from, to); + return array; + }; + + it('it should move an array element, modifying the array', () => { + expect(f(['x', 'y', 'z'], 0, 1)).toEqual(['y', 'x', 'z']); + expect(f(['x', 'y', 'z'], 1, 0)).toEqual(['y', 'x', 'z']); + expect(f(['x', 'y', 'z'], 0, 2)).toEqual(['y', 'z', 'x']); + expect(f(['x', 'y', 'z'], 2, 0)).toEqual(['z', 'x', 'y']); + expect(f(['x', 'y', 'z'], 1, 2)).toEqual(['x', 'z', 'y']); + expect(f(['x', 'y', 'z'], 2, 1)).toEqual(['x', 'z', 'y']); + expect(f(['x', 'y', 'z'], 0, 0)).toEqual(['x', 'y', 'z']); + expect(f(['x', 'y', 'z'], 1, 1)).toEqual(['x', 'y', 'z']); + expect(f(['x', 'y', 'z'], 2, 2)).toEqual(['x', 'y', 'z']); + expect(f(['a', 'b', 'c', 'd', 'e'], 4, 1)).toEqual(['a', 'e', 'b', 'c', 'd']); + expect(f(['a', 'b', 'c', 'd', 'e'], 1, 4)).toEqual(['a', 'c', 'd', 'e', 'b']); + expect(f(['a', 'b', 'c', 'd', 'e'], 1, 3)).toEqual(['a', 'c', 'd', 'b', 'e']); + }); + }); }); diff --git a/client/app/scripts/utils/array-utils.js b/client/app/scripts/utils/array-utils.js index c078913984..f20f399500 100644 --- a/client/app/scripts/utils/array-utils.js +++ b/client/app/scripts/utils/array-utils.js @@ -9,3 +9,14 @@ export function uniformSelect(array, size) { array[parseInt(index * (array.length / (size - (1 - 1e-9))), 10)] ); } + +export function insertElement(array, index, element) { + array.splice(index, 0, element); +} + +export function moveElement(array, from, to) { + if (from !== to) { + const removedElement = array.splice(from, 1)[0]; + insertElement(array, to, removedElement); + } +} From 26fc2eed58844523080841aaee43f53f82679f74 Mon Sep 17 00:00:00 2001 From: Filip Barl Date: Fri, 13 Jan 2017 12:19:57 +0100 Subject: [PATCH 2/3] Fixed row focusing for the bottom of a scrollable table --- .../node-details/node-details-table-row.js | 29 ++++++++-- .../node-details/node-details-table.js | 53 +++++++++++++------ client/app/scripts/constants/timer.js | 2 +- .../utils/__tests__/array-utils-test.js | 33 ++++++++---- client/app/scripts/utils/array-utils.js | 14 +++-- client/app/styles/main.less | 36 ++++++++++--- 6 files changed, 125 insertions(+), 42 deletions(-) diff --git a/client/app/scripts/components/node-details/node-details-table-row.js b/client/app/scripts/components/node-details/node-details-table-row.js index cba5210c1b..6ee9b919c6 100644 --- a/client/app/scripts/components/node-details/node-details-table-row.js +++ b/client/app/scripts/components/node-details/node-details-table-row.js @@ -76,17 +76,34 @@ export default class NodeDetailsTableRow extends React.Component { // user is selecting some data in the row. In this case don't trigger the onClick event which // is most likely a details panel popping open. // + this.state = { focused: false }; this.mouseDragOrigin = [0, 0]; this.saveLabelElementRef = this.saveLabelElementRef.bind(this); this.onMouseDown = this.onMouseDown.bind(this); this.onMouseUp = this.onMouseUp.bind(this); + this.onMouseEnter = this.onMouseEnter.bind(this); + this.onMouseLeave = this.onMouseLeave.bind(this); } saveLabelElementRef(ref) { this.labelElement = ref; } + onMouseEnter() { + this.setState({ focused: true }); + if (this.props.onMouseEnter) { + this.props.onMouseEnter(); + } + } + + onMouseLeave() { + this.setState({ focused: false }); + if (this.props.onMouseLeave) { + this.props.onMouseLeave(); + } + } + onMouseDown(ev) { const { pageX, pageY } = ev; this.mouseDragOrigin = [pageX, pageY]; @@ -109,18 +126,22 @@ export default class NodeDetailsTableRow extends React.Component { } render() { - const { node, nodeIdKey, topologyId, columns, onClick, selected, colStyles } = this.props; + const { node, nodeIdKey, topologyId, columns, onClick, colStyles } = this.props; const [firstColumnStyle, ...columnStyles] = colStyles; const values = renderValues(node, columns, columnStyles); const nodeId = node[nodeIdKey]; - const className = classNames('node-details-table-node', { selected }); + + const className = classNames('node-details-table-node', { + selected: this.props.selected, + focused: this.state.focused, + }); return ( ; +} + + export default class NodeDetailsTable extends React.Component { constructor(props, context) { super(props, context); + this.state = { limit: props.limit || NODE_DETAILS_DATA_ROWS_DEFAULT_LIMIT, sortedDesc: this.props.sortedDesc, sortedBy: this.props.sortedBy }; + this.focusState = {}; + this.updateSorted = this.updateSorted.bind(this); this.handleLimitClick = this.handleLimitClick.bind(this); this.onMouseLeaveRow = this.onMouseLeaveRow.bind(this); this.onMouseEnterRow = this.onMouseEnterRow.bind(this); + this.saveTableContentRef = this.saveTableContentRef.bind(this); // Use debouncing to prevent event flooding when e.g. crossing fast with mouse cursor // over the whole table. That would be expensive as each focus causes table to rerender. this.debouncedFocusRow = debounce(this.focusRow, TABLE_ROW_FOCUS_DEBOUNCE_INTERVAL); @@ -147,21 +159,23 @@ export default class NodeDetailsTable extends React.Component { } focusRow(rowIndex, node) { - this.setState({ + // Remember the focused row index, the node that was focused and + // the table content height so that we can keep the node row fixed + // without auto-scrolling happening. + // NOTE: It would be ideal to modify the real component state here, + // but that would cause whole table to rerender, which becomes to + // expensive with the current implementation if the table consists + // of 1000+ nodes. + this.focusState = { + focusedNode: node, focusedRowIndex: rowIndex, - focusedNode: node - }); - log(`Focused row ${rowIndex}`); + tableContentMinHeightConstraint: this.tableContent.scrollHeight, + }; } unfocusRow() { - if (this.state.focusedRowIndex) { - this.setState({ - focusedRowIndex: null, - focusedNode: null - }); - log('Unfocused row'); - } + // Reset the focus state + this.focusState = {}; } onMouseEnterRow(rowIndex, node) { @@ -174,6 +188,10 @@ export default class NodeDetailsTable extends React.Component { this.debouncedUnfocusRow(); } + saveTableContentRef(ref) { + this.tableContent = ref; + } + getColumnHeaders() { const columns = this.props.columns || []; return [{id: 'label', label: this.props.label}].concat(columns); @@ -181,26 +199,27 @@ export default class NodeDetailsTable extends React.Component { render() { const { nodeIdKey, columns, topologyId, onClickRow, onMouseEnter, onMouseLeave } = this.props; - const { focusedRowIndex, focusedNode } = this.state; const sortedBy = this.state.sortedBy || getDefaultSortedBy(columns, this.props.nodes); const sortedByHeader = this.getColumnHeaders().find(h => h.id === sortedBy); const sortedDesc = this.state.sortedDesc || defaultSortDesc(sortedByHeader); let nodes = getSortedNodes(this.props.nodes, sortedByHeader, sortedDesc); - if (focusedRowIndex && focusedRowIndex < nodes.length) { + + const { focusedNode, focusedRowIndex, tableContentMinHeightConstraint } = this.focusState; + if (Number.isInteger(focusedRowIndex) && focusedRowIndex < nodes.length) { const nodeRowIndex = findIndex(nodes, node => node.id === focusedNode.id); if (nodeRowIndex >= 0) { // If the focused node still exists in the table, we move it // to the hovered row, keeping the rest of the table sorted. - moveElement(nodes, nodeRowIndex, focusedRowIndex); + nodes = moveElement(nodes, nodeRowIndex, focusedRowIndex); } else { // Otherwise we insert the dead focused node there, pretending // it's still alive. That enables the users to read off all the // info they want and perhaps even open the details panel. Also, // only if we do this, we can guarantee that mouse hover will // always freeze the table row until we focus out. - insertElement(nodes, focusedRowIndex, focusedNode); + nodes = insertElement(nodes, focusedRowIndex, focusedNode); } } @@ -229,6 +248,7 @@ export default class NodeDetailsTable extends React.Component { {nodes && nodes.map((node, index) => ( @@ -245,6 +265,7 @@ export default class NodeDetailsTable extends React.Component { onMouseLeave={this.onMouseLeaveRow} topologyId={topologyId} /> ))} + {minHeightConstraint(tableContentMinHeightConstraint)} { const ArrayUtils = require('../array-utils'); @@ -7,12 +13,12 @@ describe('ArrayUtils', () => { const f = ArrayUtils.uniformSelect; it('it should select the array elements uniformly, including the endpoints', () => { + testNotMutatingArray(f, ['A', 'B', 'C', 'D', 'E'], 3); { const arr = ['x', 'y']; expect(f(arr, 3)).toEqual(['x', 'y']); expect(f(arr, 2)).toEqual(['x', 'y']); } - { const arr = ['A', 'B', 'C', 'D', 'E']; expect(f(arr, 6)).toEqual(['A', 'B', 'C', 'D', 'E']); @@ -21,7 +27,6 @@ describe('ArrayUtils', () => { expect(f(arr, 3)).toEqual(['A', 'C', 'E']); expect(f(arr, 2)).toEqual(['A', 'E']); } - { const arr = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]; expect(f(arr, 12)).toEqual([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]); @@ -36,7 +41,6 @@ describe('ArrayUtils', () => { expect(f(arr, 3)).toEqual([1, 6, 11]); expect(f(arr, 2)).toEqual([1, 11]); } - { const arr = range(1, 10001); expect(f(arr, 4)).toEqual([1, 3334, 6667, 10000]); @@ -47,12 +51,10 @@ describe('ArrayUtils', () => { }); describe('insertElement', () => { - const f = (array, index, element) => { - ArrayUtils.insertElement(array, index, element); - return array; - }; + const f = ArrayUtils.insertElement; it('it should insert an element into the array at the specified index', () => { + testNotMutatingArray(f, ['x', 'y', 'z'], 0, 'a'); expect(f(['x', 'y', 'z'], 0, 'a')).toEqual(['a', 'x', 'y', 'z']); expect(f(['x', 'y', 'z'], 1, 'a')).toEqual(['x', 'a', 'y', 'z']); expect(f(['x', 'y', 'z'], 2, 'a')).toEqual(['x', 'y', 'a', 'z']); @@ -60,13 +62,22 @@ describe('ArrayUtils', () => { }); }); + describe('removeElement', () => { + const f = ArrayUtils.removeElement; + + it('it should remove the element at the specified index from the array', () => { + testNotMutatingArray(f, ['x', 'y', 'z'], 0); + expect(f(['x', 'y', 'z'], 0)).toEqual(['y', 'z']); + expect(f(['x', 'y', 'z'], 1)).toEqual(['x', 'z']); + expect(f(['x', 'y', 'z'], 2)).toEqual(['x', 'y']); + }); + }); + describe('moveElement', () => { - const f = (array, from, to) => { - ArrayUtils.moveElement(array, from, to); - return array; - }; + const f = ArrayUtils.moveElement; it('it should move an array element, modifying the array', () => { + testNotMutatingArray(f, ['x', 'y', 'z'], 0, 1); expect(f(['x', 'y', 'z'], 0, 1)).toEqual(['y', 'x', 'z']); expect(f(['x', 'y', 'z'], 1, 0)).toEqual(['y', 'x', 'z']); expect(f(['x', 'y', 'z'], 0, 2)).toEqual(['y', 'z', 'x']); diff --git a/client/app/scripts/utils/array-utils.js b/client/app/scripts/utils/array-utils.js index f20f399500..0f962755ce 100644 --- a/client/app/scripts/utils/array-utils.js +++ b/client/app/scripts/utils/array-utils.js @@ -1,5 +1,7 @@ import { range } from 'lodash'; +// NOTE: All the array operations defined here should be non-mutating. + export function uniformSelect(array, size) { if (size > array.length) { return array; @@ -11,12 +13,16 @@ export function uniformSelect(array, size) { } export function insertElement(array, index, element) { - array.splice(index, 0, element); + return array.slice(0, index).concat([element], array.slice(index)); +} + +export function removeElement(array, index) { + return array.slice(0, index).concat(array.slice(index + 1)); } export function moveElement(array, from, to) { - if (from !== to) { - const removedElement = array.splice(from, 1)[0]; - insertElement(array, to, removedElement); + if (from === to) { + return array; } + return insertElement(removeElement(array, from), to, array[from]); } diff --git a/client/app/styles/main.less b/client/app/styles/main.less index b209661cd1..9a9c64396a 100644 --- a/client/app/styles/main.less +++ b/client/app/styles/main.less @@ -940,14 +940,21 @@ h2 { } } + tbody { + position: relative; + + .min-height-constraint { + position: absolute; + width: 0 !important; + opacity: 0; + top: 0; + } + } + &-node { font-size: 105%; line-height: 1.5; - &:hover, &.selected { - background-color: lighten(@background-color, 5%); - } - > * { padding: 0 4px; } @@ -990,6 +997,19 @@ h2 { } } +// This part sets the styles only for the 'real' node details table, not applying +// them to the nodes grid, because there we control hovering from the JS. +// NOTE: Maybe it would be nice to separate the class names between the two places +// where node tables are used - i.e. it doesn't make sense that node-details-table +// can also refer to the tables in the nodes grid. +.details-wrapper .node-details-table { + &-node { + &:hover, &.selected { + background-color: lighten(@background-color, 5%); + } + } +} + .node-control-button { .btn-opacity; padding: 6px; @@ -1785,8 +1805,9 @@ h2 { padding: 3px 4px; } + // Keeping the row height fixed is important for locking the rows on hover. .node-details-table-node, thead tr { - height: 24px; + height: 28px; } tr:nth-child(even) { @@ -1799,7 +1820,10 @@ h2 { cursor: pointer; } - tbody tr.selected, tbody tr:hover { + // We fully control hovering of the grid rows from JS, + // because we want consistent behaviour between the + // visual and row locking logic that happens on hover. + tbody tr.selected, tbody tr.focused { background-color: #d7ecf5; border: 1px solid @weave-blue; } From 1ef64cf17c9e72df72c98c3a6147846b4f58eda0 Mon Sep 17 00:00:00 2001 From: Filip Barl Date: Fri, 20 Jan 2017 12:52:03 +0100 Subject: [PATCH 3/3] Addressed David's comments --- .../node-details/node-details-table-row.js | 2 +- .../components/node-details/node-details-table.js | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/client/app/scripts/components/node-details/node-details-table-row.js b/client/app/scripts/components/node-details/node-details-table-row.js index 6ee9b919c6..0e32e4508c 100644 --- a/client/app/scripts/components/node-details/node-details-table-row.js +++ b/client/app/scripts/components/node-details/node-details-table-row.js @@ -93,7 +93,7 @@ export default class NodeDetailsTableRow extends React.Component { onMouseEnter() { this.setState({ focused: true }); if (this.props.onMouseEnter) { - this.props.onMouseEnter(); + this.props.onMouseEnter(this.props.index, this.props.node); } } diff --git a/client/app/scripts/components/node-details/node-details-table.js b/client/app/scripts/components/node-details/node-details-table.js index 388523c1cb..f1b1e93703 100644 --- a/client/app/scripts/components/node-details/node-details-table.js +++ b/client/app/scripts/components/node-details/node-details-table.js @@ -1,6 +1,6 @@ import React from 'react'; import classNames from 'classnames'; -import { find, get, union, sortBy, groupBy, concat, debounce, findIndex } from 'lodash'; +import { find, get, union, sortBy, groupBy, concat, debounce } from 'lodash'; import { NODE_DETAILS_DATA_ROWS_DEFAULT_LIMIT } from '../../constants/limits'; @@ -145,7 +145,7 @@ export default class NodeDetailsTable extends React.Component { // Use debouncing to prevent event flooding when e.g. crossing fast with mouse cursor // over the whole table. That would be expensive as each focus causes table to rerender. this.debouncedFocusRow = debounce(this.focusRow, TABLE_ROW_FOCUS_DEBOUNCE_INTERVAL); - this.debouncedUnfocusRow = debounce(this.unfocusRow, TABLE_ROW_FOCUS_DEBOUNCE_INTERVAL); + this.debouncedBlurRow = debounce(this.blurRow, TABLE_ROW_FOCUS_DEBOUNCE_INTERVAL); } updateSorted(sortedBy, sortedDesc) { @@ -173,19 +173,19 @@ export default class NodeDetailsTable extends React.Component { }; } - unfocusRow() { + blurRow() { // Reset the focus state this.focusState = {}; } onMouseEnterRow(rowIndex, node) { - this.debouncedUnfocusRow.cancel(); + this.debouncedBlurRow.cancel(); this.debouncedFocusRow(rowIndex, node); } onMouseLeaveRow() { this.debouncedFocusRow.cancel(); - this.debouncedUnfocusRow(); + this.debouncedBlurRow(); } saveTableContentRef(ref) { @@ -208,7 +208,7 @@ export default class NodeDetailsTable extends React.Component { const { focusedNode, focusedRowIndex, tableContentMinHeightConstraint } = this.focusState; if (Number.isInteger(focusedRowIndex) && focusedRowIndex < nodes.length) { - const nodeRowIndex = findIndex(nodes, node => node.id === focusedNode.id); + const nodeRowIndex = nodes.findIndex(node => node.id === focusedNode.id); if (nodeRowIndex >= 0) { // If the focused node still exists in the table, we move it // to the hovered row, keeping the rest of the table sorted. @@ -257,11 +257,12 @@ export default class NodeDetailsTable extends React.Component { renderIdCell={this.props.renderIdCell} selected={this.props.selectedNodeId === node.id} node={node} + index={index} nodeIdKey={nodeIdKey} colStyles={styles} columns={columns} onClick={onClickRow} - onMouseEnter={() => { this.onMouseEnterRow(index, node); }} + onMouseEnter={this.onMouseEnterRow} onMouseLeave={this.onMouseLeaveRow} topologyId={topologyId} /> ))}