From db018b15bc93a78a1cdfa4080e236bcf34ccb7cc Mon Sep 17 00:00:00 2001 From: Patrick Brosset Date: Wed, 10 May 2017 16:07:12 +0200 Subject: [PATCH] Bug 1359759 - 2 - Clear React warnings in GridOutline and prevent re-renders; r=gl Made the GridOutline component work with only 1 grid at a time. It already did, but in a not so obvious way. Removed the setState that happened during the render call to avoid React warnings. Cleaned up various data attribute to use the dataset property instead. Removed the mouseover/out that controled the background color of the highlighted cells. This now happens in CSS :hover, using currentColor. Avoided React warnings related to missing "key" props. Made changes to grid-inspector to avoid loops of re-renders when the highlighter would highlight a cell on hover. The component would wait for highlighter's events to dispatch store actions. Instead, we dispatch them first, then when the events come, we check if things have really changed! This way, the events will still have effect if they come from the rule-view for instance, but not if they come from the grid outline itself. MozReview-Commit-ID: 6LK8B1P8iMU --HG-- extra : rebase_source : 473ca1e1777d4e14f4f45a0b306b8153775cb1e6 --- .../inspector/grids/components/GridOutline.js | 143 ++++++++++-------- .../client/inspector/grids/grid-inspector.js | 58 +++++-- devtools/client/themes/layout.css | 3 + 3 files changed, 129 insertions(+), 75 deletions(-) diff --git a/devtools/client/inspector/grids/components/GridOutline.js b/devtools/client/inspector/grids/components/GridOutline.js index ba6d12ba0b6f..cce9025f2d20 100644 --- a/devtools/client/inspector/grids/components/GridOutline.js +++ b/devtools/client/inspector/grids/components/GridOutline.js @@ -41,7 +41,7 @@ module.exports = createClass({ getInitialState() { return { - selectedGrids: [], + selectedGrid: null, height: 0, width: 0, }; @@ -55,15 +55,42 @@ module.exports = createClass({ }, componentWillReceiveProps({ grids }) { - if (this.state.selectedGrids.length < 2) { - this.setState({ - height: 0, - width: 0, - }); + let selectedGrid = grids.find(grid => grid.highlighted); + + // Store the height of the grid container in the component state to prevent overflow + // issues. We want to store the width of the grid container as well so that the + // viewbox is only the calculated width of the grid outline. + let { width, height } = selectedGrid + ? this.getTotalWidthAndHeight(selectedGrid) + : { width: 0, height: 0 }; + + this.setState({ height, width, selectedGrid }); + }, + + /** + * Get the width and height of a given grid. + * + * @param {Object} grid + * A single grid container in the document. + * @return {Object} An object like { width, height } + */ + getTotalWidthAndHeight(grid) { + // TODO: We are drawing the first fragment since only one is currently being stored. + // In the future we will need to iterate over all fragments of a grid. + const { gridFragments } = grid; + const { rows, cols } = gridFragments[0]; + + let height = 0; + for (let i = 0; i < rows.lines.length - 1; i++) { + height += GRID_CELL_SCALE_FACTOR * (rows.tracks[i].breadth / 100); } - this.setState({ - selectedGrids: grids.filter(grid => grid.highlighted), - }); + + let width = 0; + for (let i = 0; i < cols.lines.length - 1; i++) { + width += GRID_CELL_SCALE_FACTOR * (cols.tracks[i].breadth / 100); + } + + return { width, height }; }, /** @@ -114,14 +141,12 @@ module.exports = createClass({ onShowGridAreaHighlight, onShowGridCellHighlight, } = this.props; - const name = target.getAttribute("data-grid-area-name"); - const id = target.getAttribute("data-grid-id"); - const fragmentIndex = target.getAttribute("data-grid-fragment-index"); - const color = target.getAttribute("stroke"); - const rowNumber = target.getAttribute("data-grid-row"); - const columnNumber = target.getAttribute("data-grid-column"); - - target.setAttribute("fill", color); + const name = target.dataset.gridAreaName; + const id = target.dataset.gridId; + const fragmentIndex = target.dataset.gridFragmentIndex; + const color = target.closest(".grid-cell-group").dataset.gridLineColor; + const rowNumber = target.dataset.gridRow; + const columnNumber = target.dataset.gridColumn; if (name) { onShowGridAreaHighlight(grids[id].nodeFront, name, color); @@ -140,11 +165,12 @@ module.exports = createClass({ * A single grid container in the document. */ renderGrid(grid) { - const { id, color, gridFragments } = grid; // TODO: We are drawing the first fragment since only one is currently being stored. // In the future we will need to iterate over all fragments of a grid. let gridFragmentIndex = 0; + const { id, color, gridFragments } = grid; const { rows, cols, areas } = gridFragments[gridFragmentIndex]; + const numberOfColumns = cols.lines.length - 1; const numberOfRows = rows.lines.length - 1; const rectangles = []; @@ -152,19 +178,18 @@ module.exports = createClass({ let y = 1; let width = 0; let height = 0; - // The grid outline border height/width is the total height/width of grid cells drawn. - let totalHeight = 0; - let totalWidth = 0; - // Draw the cells contained within the grid outline border. + // Draw the cells contained within the grid outline border. for (let rowNumber = 1; rowNumber <= numberOfRows; rowNumber++) { height = GRID_CELL_SCALE_FACTOR * (rows.tracks[rowNumber - 1].breadth / 100); + for (let columnNumber = 1; columnNumber <= numberOfColumns; columnNumber++) { width = GRID_CELL_SCALE_FACTOR * (cols.tracks[columnNumber - 1].breadth / 100); const gridAreaName = this.getGridAreaName(columnNumber, rowNumber, areas); const gridCell = this.renderGridCell(id, gridFragmentIndex, x, y, - rowNumber, columnNumber, color, gridAreaName, width, height); + rowNumber, columnNumber, color, gridAreaName, + width, height); rectangles.push(gridCell); x += width; @@ -172,26 +197,11 @@ module.exports = createClass({ x = 1; y += height; - totalHeight += height; - } - - // Find the total width of the grid container so we can draw the border for it - for (let columnNumber = 0; columnNumber < numberOfColumns; columnNumber++) { - totalWidth += GRID_CELL_SCALE_FACTOR * (cols.tracks[columnNumber].breadth / 100); - } - - // Store the height of the grid container in the component state to prevent overflow - // issues. We want to store the width of the grid container as well so that the - // viewbox is only the calculated width of the grid outline. - if (totalHeight > this.state.height || totalWidth > this.state.width) { - this.setState({ - height: totalHeight + 20, - width: totalWidth, - }); } // Draw a rectangle that acts as the grid outline border. - const border = this.renderGridOutlineBorder(totalWidth, totalHeight, color); + const border = this.renderGridOutlineBorder(this.state.width, this.state.height, + color); rectangles.unshift(border); return rectangles; @@ -223,7 +233,8 @@ module.exports = createClass({ gridAreaName, width, height) { return dom.rect( { - className: "grid-outline-cell", + "key": `${id}-${rowNumber}-${columnNumber}`, + "className": "grid-outline-cell", "data-grid-area-name": gridAreaName, "data-grid-fragment-index": gridFragmentIndex, "data-grid-id": id, @@ -234,31 +245,34 @@ module.exports = createClass({ width, height, fill: "none", - stroke: color, onMouseOver: this.onMouseOverCell, onMouseOut: this.onMouseLeaveCell, } ); }, - renderGridOutline(grids) { + renderGridOutline(grid) { + let { color } = grid; + return dom.g( { - className: "grid-cell-group", + "className": "grid-cell-group", + "data-grid-line-color": color, + "style": { color } }, - grids.map(grid => this.renderGrid(grid)) + this.renderGrid(grid) ); }, renderGridOutlineBorder(borderWidth, borderHeight, color) { return dom.rect( { + key: "border", className: "grid-outline-border", x: 1, y: 1, width: borderWidth, - height: borderHeight, - stroke: color, + height: borderHeight } ); }, @@ -289,6 +303,7 @@ module.exports = createClass({ gridLineNumber, lineType) { return dom.line( { + key: `${id}-${lineType}-${gridLineNumber}`, className: "grid-outline-line", "data-grid-fragment-index": gridFragmentIndex, "data-grid-id": id, @@ -306,12 +321,12 @@ module.exports = createClass({ ); }, - renderGridLines(grids) { + renderGridLines(grid) { return dom.g( { className: "grid-outline-lines", }, - grids.map(grid => this.renderLines(grid)) + this.renderLines(grid) ); }, @@ -366,10 +381,8 @@ module.exports = createClass({ onShowGridAreaHighlight, onShowGridCellHighlight, } = this.props; - const id = target.getAttribute("data-grid-id"); - const color = target.getAttribute("stroke"); - - target.setAttribute("fill", "none"); + const id = target.dataset.gridId; + const color = target.closest(".grid-cell-group").dataset.gridLineColor; onShowGridAreaHighlight(grids[id].nodeFront, null, color); onShowGridCellHighlight(grids[id].nodeFront, color); @@ -382,29 +395,29 @@ module.exports = createClass({ onMouseLeaveLine({ target }) { const { grids, onShowGridLineNamesHighlight } = this.props; - const fragmentIndex = target.getAttribute("data-grid-fragment-index"); - const id = target.getAttribute("data-grid-id"); - const color = target.getAttribute("data-grid-line-color"); + const fragmentIndex = target.dataset.gridFragmentIndex; + const id = target.dataset.gridId; + const color = target.closest(".grid-cell-group").dataset.gridLineColor; onShowGridLineNamesHighlight(grids[id].nodeFront, fragmentIndex, color); }, onMouseOverLine({ target }) { const { grids, onShowGridLineNamesHighlight } = this.props; - const fragmentIndex = target.getAttribute("data-grid-fragment-index"); - const id = target.getAttribute("data-grid-id"); - const lineNumber = target.getAttribute("data-grid-line-number"); - const type = target.getAttribute("data-grid-line-type"); - const color = target.getAttribute("data-grid-line-color"); + const fragmentIndex = target.dataset.gridFragmentIndex; + const id = target.dataset.gridId; + const lineNumber = target.dataset.gridLineNumber; + const type = target.dataset.gridLineType; + const color = target.closest(".grid-cell-group").dataset.gridLineColor; onShowGridLineNamesHighlight(grids[id].nodeFront, fragmentIndex, color, lineNumber, type); }, render() { - const { selectedGrids, height, width } = this.state; + const { selectedGrid, height, width } = this.state; - return selectedGrids.length ? + return selectedGrid ? dom.svg( { className: "grid-outline", @@ -412,8 +425,8 @@ module.exports = createClass({ height: this.getHeight(), viewBox: `${TRANSLATE_X} ${TRANSLATE_Y} ${width} ${height}`, }, - this.renderGridOutline(selectedGrids), - this.renderGridLines(selectedGrids) + this.renderGridOutline(selectedGrid), + this.renderGridLines(selectedGrid) ) : null; diff --git a/devtools/client/inspector/grids/grid-inspector.js b/devtools/client/inspector/grids/grid-inspector.js index 369c38f7e7e7..4b750016d792 100644 --- a/devtools/client/inspector/grids/grid-inspector.js +++ b/devtools/client/inspector/grids/grid-inspector.js @@ -222,6 +222,22 @@ GridInspector.prototype = { dispatch(updateShowInfiniteLines(showInfinteLines)); }, + showGridHighlighter(node, settings) { + this.lastHighlighterColor = settings.color; + this.lastHighlighterNode = node; + this.lastHighlighterState = true; + + this.highlighters.showGridHighlighter(node, settings); + }, + + toggleGridHighlighter(node, settings) { + this.lastHighlighterColor = settings.color; + this.lastHighlighterNode = node; + this.lastHighlighterState = node !== this.highlighters.gridHighlighterShown; + + this.highlighters.toggleGridHighlighter(node, settings); + }, + /** * Updates the grid panel by dispatching the new grid data. This is called when the * layout view becomes visible or the view needs to be updated with new grid data. @@ -287,8 +303,22 @@ GridInspector.prototype = { onHighlighterChange(event, nodeFront, options) { let highlighted = event === "grid-highlighter-shown"; let { color } = options; - this.store.dispatch(updateGridHighlighted(nodeFront, highlighted)); - this.store.dispatch(updateGridColor(nodeFront, color)); + + // Only tell the store that the highlighter changed if it did change. + // If we're still highlighting the same node, with the same color, no need to force + // a refresh. + if (this.lastHighlighterState !== highlighted || + this.lastHighlighterNode !== nodeFront) { + this.store.dispatch(updateGridHighlighted(nodeFront, highlighted)); + } + + if (this.lastHighlighterColor !== color || this.lastHighlighterNode !== nodeFront) { + this.store.dispatch(updateGridColor(nodeFront, color)); + } + + this.lastHighlighterColor = null; + this.lastHighlighterNode = null; + this.lastHighlighterState = null; }, /** @@ -325,7 +355,7 @@ GridInspector.prototype = { for (let grid of grids) { if (grid.nodeFront === node && grid.highlighted) { let highlighterSettings = this.getGridHighlighterSettings(node); - this.highlighters.showGridHighlighter(node, highlighterSettings); + this.showGridHighlighter(node, highlighterSettings); } } }, @@ -349,7 +379,9 @@ GridInspector.prototype = { highlighterSettings.showGridArea = gridAreaName; highlighterSettings.color = color; - this.highlighters.showGridHighlighter(node, highlighterSettings); + this.showGridHighlighter(node, highlighterSettings); + + this.store.dispatch(updateGridHighlighted(node, true)); }, /** @@ -377,7 +409,9 @@ GridInspector.prototype = { highlighterSettings.showGridCell = { gridFragmentIndex, rowNumber, columnNumber }; highlighterSettings.color = color; - this.highlighters.showGridHighlighter(node, highlighterSettings); + this.showGridHighlighter(node, highlighterSettings); + + this.store.dispatch(updateGridHighlighted(node, true)); }, /** @@ -406,10 +440,11 @@ GridInspector.prototype = { lineNumber, type }; - highlighterSettings.color = color; - this.highlighters.showGridHighlighter(node, highlighterSettings); + this.showGridHighlighter(node, highlighterSettings); + + this.store.dispatch(updateGridHighlighted(node, true)); }, /** @@ -440,7 +475,10 @@ GridInspector.prototype = { */ onToggleGridHighlighter(node) { let highlighterSettings = this.getGridHighlighterSettings(node); - this.highlighters.toggleGridHighlighter(node, highlighterSettings); + this.toggleGridHighlighter(node, highlighterSettings); + + this.store.dispatch(updateGridHighlighted(node, + node !== this.highlighters.gridHighlighterShown)); }, /** @@ -461,7 +499,7 @@ GridInspector.prototype = { for (let grid of grids) { if (grid.highlighted) { let highlighterSettings = this.getGridHighlighterSettings(grid.nodeFront); - this.highlighters.showGridHighlighter(grid.nodeFront, highlighterSettings); + this.showGridHighlighter(grid.nodeFront, highlighterSettings); } } }, @@ -484,7 +522,7 @@ GridInspector.prototype = { for (let grid of grids) { if (grid.highlighted) { let highlighterSettings = this.getGridHighlighterSettings(grid.nodeFront); - this.highlighters.showGridHighlighter(grid.nodeFront, highlighterSettings); + this.showGridHighlighter(grid.nodeFront, highlighterSettings); } } }, diff --git a/devtools/client/themes/layout.css b/devtools/client/themes/layout.css index 8052662f8a96..caec3c288bcd 100644 --- a/devtools/client/themes/layout.css +++ b/devtools/client/themes/layout.css @@ -92,18 +92,21 @@ .grid-outline-border { fill: none; + stroke: currentColor; stroke-width: 0.75; vector-effect: non-scaling-stroke; } .grid-outline-cell { pointer-events: all; + stroke: currentColor; stroke-dasharray: 0.5, 2; vector-effect: non-scaling-stroke; } .grid-outline-cell:hover { opacity: 0.45; + fill: currentColor; } .grid-outline-line {