Closed Bug 1342305 Opened 3 years ago Closed 3 years ago

Refactor grid inspector into grids/

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Similar to what we have done to the box model, we should move the grid inspector code into its own grid folder to separate out the grid code from the layout view so it can be its own individual component.
Blocks: dt-grid
Summary: Refactor grid inspector into grid/ → Refactor grid inspector into grids/
Comment on attachment 8841019 [details]
Bug 1342305 - Refactor grid inspector into its own grids folder.

https://reviewboard.mozilla.org/r/115376/#review117344

Clearing review flag for the js error in `onShowBoxModelHighlighterForNode`

::: devtools/client/inspector/grids/grids.js:37
(Diff revision 3)
> +  "#FF90FF",
> +  "#1B80FF",
> +  "#FF2647"
> +];
> +
> +function GridInspector(inspector, window) {

I would prefer if there was some mapping between the filename (grids.js) and the class (GridInspector). Why not grid-inspector.js ?

::: devtools/client/inspector/grids/grids.js:287
(Diff revision 3)
> +   *         The node to highlight.
> +   * @param  {Object} options
> +   *         Options passed to the highlighter actor.
> +   */
> +  onShowBoxModelHighlighterForNode: (nodeFront, options) => {
> +    let toolbox = this.inspector.toolbox;

error logged here: this.inspector is undefined

::: devtools/client/inspector/layout/components/App.js:32
(Diff revision 3)
>  const App = createClass({
>  
>    displayName: "App",
>  
>    propTypes: {
> -    boxModel: PropTypes.shape(Types.boxModel).isRequired,
> +    boxModel: PropTypes.shape(BoxModelTypes.boxModel).isRequired,

Just as a general comment, I think it'd be nice to simplify the props here. We could simply expect two props :
- gridProps
- boxModelProps

And forward them to the corresponding component. Here we force layout.js and App.js to merge props from both domains, only to split them right after.
Attachment #8841019 - Flags: review?(jdescottes)
Comment on attachment 8841019 [details]
Bug 1342305 - Refactor grid inspector into its own grids folder.

https://reviewboard.mozilla.org/r/115376/#review117350

I'm switching to r+ since the fix needed is really minor and doesn't require a second review pass.
Attachment #8841019 - Flags: review+
Comment on attachment 8841019 [details]
Bug 1342305 - Refactor grid inspector into its own grids folder.

https://reviewboard.mozilla.org/r/115376/#review117718
Comment on attachment 8841019 [details]
Bug 1342305 - Refactor grid inspector into its own grids folder.

https://reviewboard.mozilla.org/r/115376/#review117344

> Just as a general comment, I think it'd be nice to simplify the props here. We could simply expect two props :
> - gridProps
> - boxModelProps
> 
> And forward them to the corresponding component. Here we force layout.js and App.js to merge props from both domains, only to split them right after.

Gonna move this as follow up since I feel this will require a review to make sure I do capture what you had in mind.
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/417a8c548d39
Refactor grid inspector into its own grids folder. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/417a8c548d39
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.