Closed
Bug 1342305
Opened 7 years ago
Closed 7 years ago
Refactor grid inspector into grids/
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Summary: Refactor grid inspector into grid/ → Refactor grid inspector into grids/
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8841019 [details] Bug 1342305 - Refactor grid inspector into its own grids folder. https://reviewboard.mozilla.org/r/115376/#review117718
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/417a8c548d39
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
•