Closed
Bug 1338300
Opened 7 years ago
Closed 7 years ago
Add a color picker to the grid container listings to change the grid highlighter color
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox54 verified)
VERIFIED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: gl, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
(Keywords: DevAdvocacy, Whiteboard: [DevRel:P1])
Attachments
(3 files, 2 obsolete files)
Adds a color picker to the grid container listings to change the grid highlighter color according to the desired color choice.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8838029 [details] Bug 1338300 - part1: add color to the css grid highlighter options; https://reviewboard.mozilla.org/r/113030/#review114800 ::: devtools/server/actors/highlighters/css-grid.js:27 (Diff revision 1) > const CSS_GRID_ENABLED_PREF = "layout.css.grid.enabled"; > const ROWS = "rows"; > const COLUMNS = "cols"; > + > +// Default grid color. > +const GRID_COLOR = "#4B0082"; s/GRID_COLOR/DEFAULT_GRID_COLOR I think we can be super explicit and just rename this to be DEFAULT_GRID_COLOR and remove the comment and switch right after CSS_GRID_ENABLED_PREF. ::: devtools/server/actors/highlighters/css-grid.js:69 (Diff revision 1) > * let h = new CssGridHighlighter(env); > * h.show(node, options); > * h.hide(); > * h.destroy(); > * > * Available Options: Add color as an available option and its description. ::: devtools/server/actors/highlighters/css-grid.js:605 (Diff revision 1) > this.ctx.lineTo(endPos, linePos); > } > > - this.ctx.strokeStyle = GRID_LINES_PROPERTIES[lineType].strokeStyle; > + this.ctx.strokeStyle = this.color; > + let alpha = GRID_LINES_PROPERTIES[lineType].alpha; > + this.ctx.globalAlpha = alpha; Looks like we can save a variable declaration by just doing: this.ctx.globalAlpha = GRID_LINES_PROPERTIES[lineType].alpha;
Attachment #8838029 -
Flags: review?(gl) → review+
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8838030 [details] Bug 1338300 - part2: extract layoutview GridItem to dedicated component; https://reviewboard.mozilla.org/r/113032/#review115044 ::: devtools/client/inspector/layout/components/GridList.js:50 (Diff revision 1) > {}, > getStr("layout.overlayMultipleGrids") > ), > dom.ul( > {}, > - grids.map(grid => { > + grids.map(grid => GridItem(Object.assign({ grid }, this.props))) The way we pass the props into the GridItem is a bit inconsistent with how things are done in the rest of the components. I would rather we explicitly pass in the individual props that GridItem requires for now. I am looking at ways we can mitigate the pain of having to pass down the props from the root component to the children, but for now, I would prefer if we remind consistent. GridItem({ grid, onSetGridOverlayColor, ..., })
Attachment #8838030 -
Flags: review?(gl) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8838029 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8838030 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8839673 [details] Bug 1338300 - part1: add color to the css grid highlighter options; https://reviewboard.mozilla.org/r/114196/#review115760 ::: devtools/server/actors/highlighters/css-grid.js:69 (Diff revision 1) > * h.show(node, options); > * h.hide(); > * h.destroy(); > * > * Available Options: > + * - color {String} This should be: - color(colorValue) @param {String} colorValue
Attachment #8839673 -
Flags: review?(gl) → review+
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8839674 [details] Bug 1338300 - part2: extract layoutview GridItem to dedicated component; https://reviewboard.mozilla.org/r/114198/#review115768
Attachment #8839674 -
Flags: review?(gl) → review+
Reporter | ||
Updated•7 years ago
|
Flags: qe-verify+
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8838031 [details] Bug 1338300 - part3: add colorpicker to update grid overlay color; https://reviewboard.mozilla.org/r/113034/#review115808 ::: devtools/client/inspector/layout/actions/grids.js:27 (Diff revision 3) > + */ > + updateGridColor(nodeFront, color) { > + return { > + type: UPDATE_GRID_COLOR, > + nodeFront, > + color, Move color above nodeFront ::: devtools/client/inspector/layout/components/App.js:32 (Diff revision 3) > propTypes: { > boxModel: PropTypes.shape(Types.boxModel).isRequired, > grids: PropTypes.arrayOf(PropTypes.shape(Types.grid)).isRequired, > highlighterSettings: PropTypes.shape(Types.highlighterSettings).isRequired, > showBoxModelProperties: PropTypes.bool.isRequired, > + getSwatchColorPickerTooltip: PropTypes.func.isRequired, Seems like the propTypes somehow got out of alphabetically ordered (with the exception that handlers are always listed at the bottom of propTypes list). Move getSwatchColorPickerTooltip above grids. ::: devtools/client/inspector/layout/components/App.js:33 (Diff revision 3) > boxModel: PropTypes.shape(Types.boxModel).isRequired, > grids: PropTypes.arrayOf(PropTypes.shape(Types.grid)).isRequired, > highlighterSettings: PropTypes.shape(Types.highlighterSettings).isRequired, > showBoxModelProperties: PropTypes.bool.isRequired, > + getSwatchColorPickerTooltip: PropTypes.func.isRequired, > onShowBoxModelEditor: PropTypes.func.isRequired, onShowBoxModelEditor needs to swap with onHideBoxModelHighlighter ::: devtools/client/inspector/layout/components/App.js:36 (Diff revision 3) > showBoxModelProperties: PropTypes.bool.isRequired, > + getSwatchColorPickerTooltip: PropTypes.func.isRequired, > onShowBoxModelEditor: PropTypes.func.isRequired, > onHideBoxModelHighlighter: PropTypes.func.isRequired, > onShowBoxModelHighlighter: PropTypes.func.isRequired, > + onSetGridOverlayColor: PropTypes.func.isRequired, onSetGridOverlayColor needs to appear above onShowBoxModelEditor to maintain the alphabetical order of the handlers. ::: devtools/client/inspector/layout/components/Grid.js:23 (Diff revision 3) > displayName: "Grid", > > propTypes: { > grids: PropTypes.arrayOf(PropTypes.shape(Types.grid)).isRequired, > highlighterSettings: PropTypes.shape(Types.highlighterSettings).isRequired, > + getSwatchColorPickerTooltip: PropTypes.func.isRequired, Move getSwatchColorPickerTooltip above grids. ::: devtools/client/inspector/layout/components/Grid.js:36 (Diff revision 3) > > render() { > let { > grids, > highlighterSettings, > + getSwatchColorPickerTooltip, Same as above. ::: devtools/client/inspector/layout/components/Grid.js:50 (Diff revision 3) > { > id: "layout-grid-container", > }, > GridList({ > grids, > + getSwatchColorPickerTooltip, Same as above. ::: devtools/client/inspector/layout/components/GridItem.js:9 (Diff revision 3) > > "use strict"; > > const { addons, createClass, DOM: dom, PropTypes } = > require("devtools/client/shared/vendor/react"); > +const {findDOMNode} = require("devtools/client/shared/vendor/react-dom"); s/{findDOMNode}/{ findDOMNode } to be consistent ::: devtools/client/inspector/layout/components/GridItem.js:19 (Diff revision 3) > > displayName: "GridItem", > > propTypes: { > grid: PropTypes.shape(Types.grid).isRequired, > + getSwatchColorPickerTooltip: PropTypes.func.isRequired, Move above grid. ::: devtools/client/inspector/layout/components/GridItem.js:32 (Diff revision 3) > + let tooltip = this.props.getSwatchColorPickerTooltip(); > + let swatchEl = findDOMNode(this).querySelector(".grid-color-swatch"); > + > + let previousColor; > + tooltip.addSwatch(swatchEl, { > + onShow: () => { Reorder these handlers alphabetically: onCommit, onShow, onPreview, onRevert ::: devtools/client/inspector/layout/components/GridItem.js:108 (Diff revision 3) > + title: grid.color, > + } > + ), > + // The SwatchColorPicker relies on the nextSibling of the swatch element to apply > + // the selected color. This is why we use a span in display: none for now. > + // Ideally we should modify the SwatchColorPickerTooltip to bypass this requirement. File a bug prior to landing. ::: devtools/client/inspector/layout/components/GridList.js:21 (Diff revision 3) > > displayName: "GridList", > > propTypes: { > grids: PropTypes.arrayOf(PropTypes.shape(Types.grid)).isRequired, > + getSwatchColorPickerTooltip: PropTypes.func.isRequired, Move above grids ::: devtools/client/inspector/layout/components/GridList.js:31 (Diff revision 3) > mixins: [ addons.PureRenderMixin ], > > render() { > let { > grids, > + getSwatchColorPickerTooltip, Same as above ::: devtools/client/inspector/layout/components/GridList.js:48 (Diff revision 3) > ), > dom.ul( > {}, > grids.map(grid => GridItem({ > grid, > + getSwatchColorPickerTooltip, Move above grid ::: devtools/client/inspector/layout/layout.js:111 (Diff revision 3) > showBoxModelProperties: true, > > /** > + * Retrieve the shared SwatchColorPicker instance. > + */ > + getSwatchColorPickerTooltip: () => { Move this above showBoxModelProperties ::: devtools/client/inspector/layout/layout.js:219 (Diff revision 3) > - let { highlighterSettings } = this.store.getState(); > + let highlighterSettings = this.getHighlighterSettings(node); > this.highlighters.toggleGridHighlighter(node, highlighterSettings); > }, > > /** > * Handler for a change in the show grid line numbers checkbox in the This JSDoc needs to be revised for onSetGridOverlayColor ::: devtools/client/inspector/layout/layout.js:227 (Diff revision 3) > * grids currently highlighted. > * > * @param {Boolean} enabled > * Whether or not the grid highlighter should show the grid line numbers. > */ > + onSetGridOverlayColor: (node, color) => { Move this before onShowBoxModelEditor ::: devtools/client/inspector/layout/layout.js:243 (Diff revision 3) > + } > + }, > + > + /** > + * Handler for a change in the show grid line numbers checkbox in the > + * GridDisplaySettings component. TOggles on/off the option to show the grid line s/TOggles/Toggles ::: devtools/client/inspector/layout/layout.js:458 (Diff revision 3) > grids.push({ > id: i, > gridFragments: grid.gridFragments, > highlighted: nodeFront == this.highlighters.gridHighlighterShown, > nodeFront, > + color, Move after id ::: devtools/client/inspector/layout/layout.js:527 (Diff revision 3) > this.updateBoxModel(); > this.updateGridPanel(); > }, > > + /** > + * Create a highlighter settings object for the provided nodeFront. Prefer it to state: Create a grid highlighter settings object for the provided grid container element. Also add the proper @param JSDocs ::: devtools/client/inspector/layout/layout.js:529 (Diff revision 3) > }, > > + /** > + * Create a highlighter settings object for the provided nodeFront. > + */ > + getHighlighterSettings(nodeFront) { Move this after destroy() ::: devtools/client/inspector/layout/layout.js:529 (Diff revision 3) > }, > > + /** > + * Create a highlighter settings object for the provided nodeFront. > + */ > + getHighlighterSettings(nodeFront) { The highlighterSettings was originally treated as a global setting object. The object that is returned in this context is not really the same anymore. We should rename this to getGridHighlighterSettings. ::: devtools/client/inspector/layout/layout.js:542 (Diff revision 3) > + color > + }); > + }, > + > + /** > + * Returns the color set for the grid highlighter associated with provided nodeFront. s/with/with the Also, add the @params JSDocs ::: devtools/client/inspector/layout/layout.js:544 (Diff revision 3) > + }, > + > + /** > + * Returns the color set for the grid highlighter associated with provided nodeFront. > + */ > + getGridColorForNodeFront(nodeFront) { Move above getHighlighterSettings/getGridHighlighterSettings ::: devtools/client/inspector/layout/layout.js:545 (Diff revision 3) > + > + /** > + * Returns the color set for the grid highlighter associated with provided nodeFront. > + */ > + getGridColorForNodeFront(nodeFront) { > + let { grids } = this.store.getState(); Add a new line before the for loop block. ::: devtools/client/inspector/layout/layout.js:550 (Diff revision 3) > + let { grids } = this.store.getState(); > + for (let grid of grids) { > + if (grid.nodeFront === nodeFront) { > + return grid.color; > + } > + } Add a new line after the for block ::: devtools/client/inspector/layout/reducers/grids.js:16 (Diff revision 3) > } = require("../actions/index"); > > const INITIAL_GRIDS = []; > > let reducers = { > + [UPDATE_GRID_COLOR](grids, { nodeFront, color }) { Add a new line before this. ::: devtools/client/inspector/layout/types.js:37 (Diff revision 3) > > // The node front of the grid container > nodeFront: PropTypes.object, > > + // The color for the grid overlay highlighter > + color: PropTypes.string, Move this above id to keep it alphabetically ordered. ::: devtools/client/themes/layout.css:57 (Diff revision 3) > text-align: center; > padding: 0.5em; > } > + > +/** > + * GridItem display in listing s/GridItem display in listing/Grid Items ::: devtools/client/themes/layout.css:73 (Diff revision 3) > + > +.grid-color-swatch { > + width: 12px; > + height: 12px; > + margin-left: 5px; > + Remove empty line. ::: devtools/client/themes/layout.css:75 (Diff revision 3) > + width: 12px; > + height: 12px; > + margin-left: 5px; > + > + border: 1px solid var(--theme-highlight-gray); > + border-radius: 100%; Add: cursor: pointer; ::: devtools/client/themes/layout.css:75 (Diff revision 3) > + width: 12px; > + height: 12px; > + margin-left: 5px; > + > + border: 1px solid var(--theme-highlight-gray); > + border-radius: 100%; This should be equivalent to border-radius: 50% if anything I think border-radius: 50% is actually what we want to use for making circles.
Attachment #8838031 -
Flags: review?(gl) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838031 [details] Bug 1338300 - part3: add colorpicker to update grid overlay color; https://reviewboard.mozilla.org/r/113034/#review115808 Thanks for the review. We really need to discuss about the linting issues (properties order, blank lines etc.)! They need to be checked with actual linting rules and not at review time. I "try" to follow whatever seems to be the rule in the files I edit. But exceptions such as "handlers are at the end", "type is first in actions", "id is first" make it really hard to see a pattern and apply it :( Could we stick to real alphabetical order, no exceptions? And add the eslint sort-keys rule to enforce it? (will need to be ignored on some lines to preserve react/sort-comp, but that should be manageable). I understand you'd prefer to have exceptions in some cases, but that can come later with a custom plugin. > Move above grid. So here my reasoning was that in layout files, functions seemed to be at the end of props, regardless of the alphabetical order. > Reorder these handlers alphabetically: onCommit, onShow, onPreview, onRevert The other order also makes sense as it matches the lifecycle of the widget. Without linting here, it's hard to say why we should pick one over the other. > Move above grids Again, to me looked like functions were at the end. > Move this above id to keep it alphabetically ordered. grid and highlighted should also be above id in this case
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838031 [details] Bug 1338300 - part3: add colorpicker to update grid overlay color; https://reviewboard.mozilla.org/r/113034/#review115808 > File a bug prior to landing. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1341578
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8839673 [details] Bug 1338300 - part1: add color to the css grid highlighter options; https://reviewboard.mozilla.org/r/114196/#review115760 > This should be: > - color(colorValue) > @param {String} colorValue I updated it, but I don't understand why this is documented this way. We are not documenting functions with parameters. Other grids (box-model for instance) don't follow this pattern.
Assignee | ||
Comment 25•7 years ago
|
||
Let's wait for try and land: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a64e1c8619613740740b9bafafe6c2299ccb2fd5
Comment hidden (mozreview-request) |
Comment 27•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/73ac9c88604a part1: add color to the css grid highlighter options;r=gl https://hg.mozilla.org/integration/autoland/rev/8ece5e93369b part2: extract layoutview GridItem to dedicated component;r=gl https://hg.mozilla.org/integration/autoland/rev/d84d57475e82 part3: add colorpicker to update grid overlay color;r=gl
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73ac9c88604a https://hg.mozilla.org/mozilla-central/rev/8ece5e93369b https://hg.mozilla.org/mozilla-central/rev/d84d57475e82
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 29•7 years ago
|
||
Verified fixed using the latest Nightly 54.0a1 (2017-03-06) on Ubuntu 16.04, Mac OS X 10.10 and Windows 10 x64.
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Flags: qe-verify+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•