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)

defect

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.
Yes, please. Thanks.
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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+
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+
Attachment #8838029 - Attachment is obsolete: true
Attachment #8838030 - Attachment is obsolete: true
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+
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+
Flags: qe-verify+
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 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 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 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.
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
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
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.