Closed
Bug 1337235
Opened 7 years ago
Closed 7 years ago
Add mouseover interaction for the grid outline to display grid area highlights
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox54 fixed)
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: micah, Assigned: micah)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
We need to implement mouseover interactions for the grid outline. This bug should: 1. Figure out the row/col of the mouseover using the grid fragment data. 2. Figure out if there is a grid area, if yes then show the grid highlight.
Updated•7 years ago
|
Assignee: nobody → tigleym
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8840275 [details] Bug 1337235 - Mouseover interaction for grid outline. https://reviewboard.mozilla.org/r/114762/#review116294
Attachment #8840275 -
Flags: review?(gl)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8840275 -
Flags: review?(gl)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8840322 [details] Bug1337235 - Mouseover interactions for grid outline. https://reviewboard.mozilla.org/r/114828/#review116972 ::: devtools/client/inspector/layout/components/GridOutline.js:63 (Diff revision 1) > } > ); > }, > > /** > + * getGridAreaName - Determines if grid cell is part of a grid area and Apply the JSDoc formatting that you should be used to from the grid outline patch. ::: devtools/client/inspector/layout/components/GridOutline.js:63 (Diff revision 1) > } > ); > }, > > /** > + * getGridAreaName - Determines if grid cell is part of a grid area and Comment should be: Returns the grid area name if the given grid cell is part of a grid area. ::: devtools/client/inspector/layout/components/GridOutline.js:101 (Diff revision 1) > - highlightedGrids.push(grid.gridFragments[0]); > } > return highlightedGrids; > }, > > - renderCell(fragment, columnNumber, rowNumber) { > + renderCell(fragment, columnNumber, rowNumber, areas, nodeId) { This is mostly a rebasing thing since it differs from the latest grid outline patch, but I don't think we need the fragment here. ::: devtools/client/inspector/layout/components/GridOutline.js:102 (Diff revision 1) > } > return highlightedGrids; > }, > > - renderCell(fragment, columnNumber, rowNumber) { > + renderCell(fragment, columnNumber, rowNumber, areas, nodeId) { > + const name = this.getGridAreaName(fragment, areas); The grid fragment object is different from {row, col} as an object. I would just pass in the individual row and col to not confuse things. ::: devtools/client/inspector/layout/components/GridOutline.js:107 (Diff revision 1) > + const name = this.getGridAreaName(fragment, areas); > return dom.rect( > { > className: "grid-outline-cell", > + "data-nodeId": nodeId, > + "data-name": name, Put data-name before data-nodeId ::: devtools/client/inspector/layout/components/GridOutline.js:119 (Diff revision 1) > ); > }, > > - renderFragment(gridFragment) { > - const { rows, cols } = gridFragment; > + renderFragment({gridFragments, id}) { > + // TODO: We are drawing the first fragment since only one is currently being stored. > + // In future we will need to iterate over all fragments of a grid. In the future,* ::: devtools/client/inspector/layout/components/GridOutline.js:151 (Diff revision 1) > > return rectangles; > }, > > onMouseOverCell({ target }) { > - // TODO: We will want onMouseOver interactions with the grid cells, which is addressed > + const { grids, onShowGridAreaHighlight } = this.props; const { grids, ... } = this.props; ::: devtools/client/inspector/layout/components/GridOutline.js:153 (Diff revision 1) > }, > > onMouseOverCell({ target }) { > - // TODO: We will want onMouseOver interactions with the grid cells, which is addressed > - // in Bug 1337235 > + const { grids, onShowGridAreaHighlight } = this.props; > + const nodeId = target.getAttribute("data-nodeId"); > + const name = target.getAttribute("data-name"); Move this above nodeId ::: devtools/client/inspector/layout/layout.js:229 (Diff revision 1) > > toolbox.highlighterUtils.highlightNodeFront(nodeFront, options); > }, > > /** > + * Handler for a change in the grid area being highlighted. We first describe when this handler is called: Handler for a mouseover in the grid outline of a grid area. Highlights the grid area in the CSS Grid Highlighter for the given grid area. ::: devtools/client/inspector/layout/layout.js:231 (Diff revision 1) > }, > > /** > + * Handler for a change in the grid area being highlighted. > + * Shows the grid area currently being highlighted. > + * @param {NodeFront} node Format the JSDoc similar to what we have done before.
Attachment #8840322 -
Flags: review?(gl)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8840322 -
Attachment is obsolete: true
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8840275 [details] Bug 1337235 - Mouseover interaction for grid outline. https://reviewboard.mozilla.org/r/114762/#review117004 ::: devtools/client/inspector/layout/components/GridOutline.js:44 (Diff revision 4) > selectedGrids: grids.filter(grid => grid.highlighted), > }); > }, > > - renderGridCell(columnNumber, rowNumber, color) { > + /** > + * Returns the grid area name if the given grid cell is part of a grid area. Each * needs 1 more space indentation. Should align with /* ::: devtools/client/inspector/layout/components/GridOutline.js:52 (Diff revision 4) > + * The column number of the grid cell. > + * @param {Number} row > + * The row number of the grid cell. > + * @param {Array} areas > + * List of grid areas stored on grid fragment > + * @returns {String} s/@returns/@return. {String} should align with {Array} so remove one space. after @return. ::: devtools/client/inspector/layout/components/GridOutline.js:73 (Diff revision 4) > + * > + * @param {Number} x > + * The x-position of the grid cell. > + * @param {Number} y > + * The y-position of the grid cell. > + * @param {Number} col s/col/columnNumber ::: devtools/client/inspector/layout/components/GridOutline.js:75 (Diff revision 4) > + * The x-position of the grid cell. > + * @param {Number} y > + * The y-position of the grid cell. > + * @param {Number} col > + * The column number of the grid cell. > + * @param {Number} row s/row/rowNumber ::: devtools/client/inspector/layout/components/GridOutline.js:78 (Diff revision 4) > + * @param {Number} col > + * The column number of the grid cell. > + * @param {Number} row > + * The row number of the grid cell. > + * @param {Array} areas > + * List of grid areas stored on grid fragment Add with a period. s/List/Array s/grid areas/grid areas data s/on/in the ::: devtools/client/inspector/layout/components/GridOutline.js:82 (Diff revision 4) > + * @param {Array} areas > + * List of grid areas stored on grid fragment > + */ > + renderGridCell(x, y, columnNumber, rowNumber, color, areas) { > + const name = this.getGridAreaName(columnNumber, rowNumber, areas); > return dom.rect( I think we have a grid key that we can store in a data-id attribute to make getting the grid and its nodefront easier in onShowGridAreaHighlight so we don't have to traverse through the list of grids as well. ::: devtools/client/inspector/layout/components/GridOutline.js:85 (Diff revision 4) > + renderGridCell(x, y, columnNumber, rowNumber, color, areas) { > + const name = this.getGridAreaName(columnNumber, rowNumber, areas); > return dom.rect( > { > className: "grid-outline-cell", > - x: columnNumber, > + "data-name": name, s/data-name/data-grid-area-name ::: devtools/client/inspector/layout/components/GridOutline.js:164 (Diff revision 4) > + const name = target.getAttribute("data-name"); > + const color = target.getAttribute("stroke"); > + > + target.setAttribute("fill", color); > + > + onShowGridAreaHighlight(name, color); We need to make sure we aren't still highlighting the grid area when we leave a cell that has a grid area to a cell that doesn't have a grid area. ::: devtools/client/inspector/layout/layout.js:105 (Diff revision 4) > return this.swatchColorPickerTooltip; > }, > > /** > * Set the inspector selection. > * @param {NodeFront} nodeFront Add a new line before @param here while you are making fixes. ::: devtools/client/inspector/layout/layout.js:153 (Diff revision 4) > > onShowBoxModelEditor, > onShowBoxModelHighlighter, > > /** > * Shows the box-model highlighter on the element corresponding to the provided Ah, I see the indentation for this is also messed up. We need to remove a space before the *. Can you fix this while we are at it. ::: devtools/client/inspector/layout/layout.js:167 (Diff revision 4) > let toolbox = this.inspector.toolbox; > toolbox.highlighterUtils.highlightNodeFront(nodeFront, options); > }, > > /** > + * Handler for a mouseover in the grid outline of a grid area. * needs 1 more space indented. ::: devtools/client/inspector/layout/layout.js:167 (Diff revision 4) > let toolbox = this.inspector.toolbox; > toolbox.highlighterUtils.highlightNodeFront(nodeFront, options); > }, > > /** > + * Handler for a mouseover in the grid outline of a grid area. This isn't really a handler for any typical events. So, we should just be describing its functionality which is stated in the second sentence of the comments. We can remove this handler sentence.
Attachment #8840275 -
Flags: review?(gl)
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8840275 [details] Bug 1337235 - Mouseover interaction for grid outline. https://reviewboard.mozilla.org/r/114762/#review117246
Attachment #8840275 -
Flags: review?(gl)
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8840275 [details] Bug 1337235 - Mouseover interaction for grid outline. https://reviewboard.mozilla.org/r/114762/#review117556
Attachment #8840275 -
Flags: review?(gl) → review+
Comment 13•7 years ago
|
||
Attachment #8842161 -
Flags: review+
Comment 14•7 years ago
|
||
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a15dfecc9fa Mouseover interaction for grid outline. r=gl
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0a15dfecc9fa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•