Add initial app boilerplate for creating a new layout panel

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Inspector
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

a year ago
The purpose of this bug is to add a new layout panel to the Inspector sidebar. Hidden by default for the purpose of working on the CSS Grid panel within the layout panel. This bug will add the necessary react/redux boilerplate needed to start the CSS Grid panel project.
(Assignee)

Comment 1

a year ago
Created attachment 8795995 [details] [diff] [review]
1305786.patch

Honza, I am trying to start a new redux/react app inside an inspector tab panel. 

Currently, running into the following error:
Invariant Violation: addComponentAsRefTo(...): Only a ReactOwner can have refs. You might be adding a ref to a component that was not created inside a component's `render` method, or you have multiple copies of React loaded (details: https://fb.me/react-refs-must-have-owner).

I was wondering if you know what may be causing or the best way to approach what I am trying to achieve. The first papercut I ran into was the need to use the browser loader, so, I am using the React loaded already in the the inspector panel. 

I suspect I actually need to start redux/react app at the top level of the inspector panel when the inspector tab panel is being created. I know I can get this working in an iframe, but that is something we want to avoid.
Attachment #8795995 - Flags: feedback?(odvarko)
Created attachment 8796144 [details] [diff] [review]
bug1305786-loader.patch

Patch avoiding React double load attached.

I am seeing two issues in your patch:

1) You need to use browserLoader to load top level modules. There is one available in the InspectorPanel through a getter (coming from the Toolbox) so, it can be used.

2) BrowserLoader maintains a list of hard-coded directories from which it's loading components. And since you introduced a new App component (devtools/client/inspector/layout/) the dir must be included.

This is not nice, but we need to live with that (I guess till the entire DevTools are React).

I appended "resource://devtools/client/inspector/" dir to the list, but if it's ok for you to put the App component into (for now) empty 'layout/components' directory it should work without a change. 

There is regexp, in addition to the hard-coded list:
https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/devtools/client/shared/browser-loader.js#27

And that should be enough if all components are in 'layout/components' dir.

Honza
Comment on attachment 8795995 [details] [diff] [review]
1305786.patch

Just clearing the feedback flag.

Honza
Attachment #8795995 - Flags: feedback?(odvarko)
(Assignee)

Comment 4

a year ago
Created attachment 8796420 [details] [diff] [review]
1305786.patch [1.0]
Attachment #8795995 - Attachment is obsolete: true
Attachment #8796144 - Attachment is obsolete: true
Attachment #8796420 - Flags: review?(odvarko)
Comment on attachment 8796420 [details] [diff] [review]
1305786.patch [1.0]

Review of attachment 8796420 [details] [diff] [review]:
-----------------------------------------------------------------

Look great in general, just some inline comments.

Honza

::: devtools/client/inspector/inspector-panel.js
@@ +23,5 @@
>  const {ComputedViewTool} = require("devtools/client/inspector/computed/computed");
>  const {FontInspector} = require("devtools/client/inspector/fonts/fonts");
>  const {HTMLBreadcrumbs} = require("devtools/client/inspector/breadcrumbs");
>  const {InspectorSearch} = require("devtools/client/inspector/inspector-search");
> +const {LayoutViewTool} = require("devtools/client/inspector/layout/layout");

inspector-panel.js has been renamed to inspector.js so, there are conflicts when applying this patch.

::: devtools/client/inspector/layout/layout.js
@@ +18,5 @@
> +    const React = this.inspector.React;
> +    const ReactDOM = this.inspector.ReactDOM;
> +    const browserRequire = this.inspector.browserRequire;
> +
> +    const { Provider } = browserRequire("devtools/client/shared/vendor/react-redux");

I know this comes from the patch I created, but I think it would be nice if 'react-redux' binding module is accessible (and shared) just like 'react' or 'react-dom' modules (Toolbox -> InspectorPanel).

See: 
https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/framework/toolbox.js#L482
https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/inspector/inspector.js#L412

::: devtools/client/inspector/layout/reducers/grids.js
@@ +16,5 @@
> +  let reducer = reducers[action.type];
> +  if (!reducer) {
> +    return grids;
> +  }
> +  return reducer(grids, action);

What do you compose reducers this way?
I am also seeing index.js file (in reducers dir) that should
do this job using 'combineReducers' no?

E.g.

export default combineReducers({
  grids,
  secondReducer: myReducer,
  etc,
});
Attachment #8796420 - Flags: review?(odvarko)
(Assignee)

Comment 6

a year ago
Created attachment 8796681 [details] [diff] [review]
1305786.patch [2.0]

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> Comment on attachment 8796420 [details] [diff] [review]
> 1305786.patch [1.0]
> 
> Review of attachment 8796420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Look great in general, just some inline comments.
> 
> Honza
> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +23,5 @@
> >  const {ComputedViewTool} = require("devtools/client/inspector/computed/computed");
> >  const {FontInspector} = require("devtools/client/inspector/fonts/fonts");
> >  const {HTMLBreadcrumbs} = require("devtools/client/inspector/breadcrumbs");
> >  const {InspectorSearch} = require("devtools/client/inspector/inspector-search");
> > +const {LayoutViewTool} = require("devtools/client/inspector/layout/layout");
> 
> inspector-panel.js has been renamed to inspector.js so, there are conflicts
> when applying this patch.
> 

Fixed.

> ::: devtools/client/inspector/layout/layout.js
> @@ +18,5 @@
> > +    const React = this.inspector.React;
> > +    const ReactDOM = this.inspector.ReactDOM;
> > +    const browserRequire = this.inspector.browserRequire;
> > +
> > +    const { Provider } = browserRequire("devtools/client/shared/vendor/react-redux");
> 
> I know this comes from the patch I created, but I think it would be nice if
> 'react-redux' binding module is accessible (and shared) just like 'react' or
> 'react-dom' modules (Toolbox -> InspectorPanel).
> 
> See: 
> https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/framework/
> toolbox.js#L482
> https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/inspector/
> inspector.js#L412
> 

Fixed. Added ReactRedux to toolbox.js and inspector.js

> ::: devtools/client/inspector/layout/reducers/grids.js
> @@ +16,5 @@
> > +  let reducer = reducers[action.type];
> > +  if (!reducer) {
> > +    return grids;
> > +  }
> > +  return reducer(grids, action);
> 
> What do you compose reducers this way?
> I am also seeing index.js file (in reducers dir) that should
> do this job using 'combineReducers' no?
> 
> E.g.
> 
> export default combineReducers({
>   grids,
>   secondReducer: myReducer,
>   etc,
> });

I am following the way we compose reducers in the responsive.html project. See responsive.html/reducers/. It is indeed different from what you will see in the DOM view, console and debugger. I plan to stick to the methodology used in responsive.html
Attachment #8796420 - Attachment is obsolete: true
Attachment #8796681 - Flags: review?(odvarko)
(Assignee)

Updated

a year ago
Blocks: 1181227
Keywords: dev-doc-needed
(Assignee)

Updated

a year ago
Keywords: dev-doc-needed
Summary: Create a new layout panel in the Inspector → Add initial app boilerplate for creating a new layout panel
(Assignee)

Comment 7

a year ago
Created attachment 8796782 [details] [diff] [review]
1305786.patch [3.0]
Attachment #8796681 - Attachment is obsolete: true
Attachment #8796681 - Flags: review?(odvarko)
Attachment #8796782 - Flags: review?(odvarko)
Comment on attachment 8796782 [details] [diff] [review]
1305786.patch [3.0]

Review of attachment 8796782 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now!

Honza
Attachment #8796782 - Flags: review?(odvarko) → review+
(Assignee)

Comment 9

a year ago
https://hg.mozilla.org/integration/fx-team/rev/7aeb526759fe3a57a87f0af96a35d64b4f23f4ab
Bug 1305786 -  Add initial react/redux boilerplate for creating a new layout panel r=honza

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7aeb526759fe
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Comment 11

a year ago
(In reply to Carsten Book [:Tomcat] from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/7aeb526759fe
> 
> +# LOCALIZATION NOTE (inspector.sidebar.computedViewTitle):
> +# This is the title shown in a tab in the side panel of the Inspector panel
> +# that corresponds to the tool displaying layout information defined in the page.
> +inspector.sidebar.layoutViewTitle=Layout

Note and entity mismatch.

Additionally, "Layout" and "Box Model" appear to be inconsistent and cause confusion, looking at various locales, likely because it used to be called Box Model and changed silently without entity change.
Can this be fixed?

https://hg.mozilla.org/mozilla-central/rev/f249501590e6
https://hg.mozilla.org/mozilla-central/rev/41f5b98f7b55
https://transvision.mozfr.org/string/?entity=devtools/client/inspector.properties:inspector.sidebar.layoutViewTitle&repo=aurora
(In reply to Ton from comment #11)
> 
> Note and entity mismatch.
> 
> Additionally, "Layout" and "Box Model" appear to be inconsistent and cause
> confusion, looking at various locales, likely because it used to be called
> Box Model and changed silently without entity change.
> Can this be fixed?
> 

Thanks for the heads up! Will fix this in Bug 1321504.
You need to log in before you can comment on or make changes to this bug.