From da1c6f53e37e18d02aeb5501563fd166308d7b9e Mon Sep 17 00:00:00 2001 From: Filip Barl Date: Fri, 13 Jan 2017 12:19:57 +0100 Subject: [PATCH] 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; }