JSON Viewer: use shared tree component

RESOLVED FIXED in Firefox 48

Status

P2
normal
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: Honza, Assigned: Honza)

Tracking

unspecified
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(4 attachments, 2 obsolete attachments)

JSON Viewer should reuse shared tree component from:
devtools/client/shared/components/tree

Honza
Depends on: 1257173
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Created attachment 8732181 [details] [diff] [review]
bug1256757-tree-amd.patch

Support for AMD in tree modules (i.e. using define() method in: devtools/client/shared/components/tree directory)

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8732181 - Flags: review?(jryans)
Created attachment 8732183 [details] [diff] [review]
bug1256757-json-tree.patch

Using shared tree from: devtools/shared/components/tree and removing the old one.

Honza
Attachment #8732183 - Flags: review?(jryans)
Created attachment 8732184 [details] [diff] [review]
bug1256757-expose-common-styles.patch

Exposing common.css through resource:// protocol (just like variables.css) and using it in JSON Viewer.

Honza
Attachment #8732184 - Flags: review?(jryans)
Created attachment 8732196 [details] [diff] [review]
bug1256757-tests.patch

Tests updated

Honza
Attachment #8732196 - Flags: review?(jryans)
Patches should be applied in this order:
1) bug1256757-tree-amd.patch
2) bug1256757-json-tree.patch
3) bug1256757-expose-common-styles.patch
4) bug1256757-tests.patch

Honza
Comment on attachment 8732181 [details] [diff] [review]
bug1256757-tree-amd.patch

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

Let's add the comment:

// Make this available to both AMD and CJS environments

above the define line.  It's not obvious from these files that it will work in CJS without changes due to our loader support, so I think the comment is helpful to have.
Attachment #8732181 - Flags: review?(jryans) → review+
Comment on attachment 8732183 [details] [diff] [review]
bug1256757-json-tree.patch

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

::: devtools/client/jsonview/components/json-panel.js
@@ +6,5 @@
>  
>  "use strict";
>  
>  define(function(require, exports, module) {
>    const React = require("devtools/client/shared/vendor/react");

I think it's more common in other tools to destructure from React, like:

const { DOM: dom, createFactory } = require("devtools/client/shared/vendor/react");

Probably best for this to match that.  See also the new style guide[1].

[1]: https://wiki.mozilla.org/DevTools/CodingStandards#React_.26_Redux
Attachment #8732183 - Flags: review?(jryans) → review+
Created attachment 8732818 [details] [diff] [review]
bug1256757-tree-amd.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8732181 [details] [diff] [review]
> bug1256757-tree-amd.patch
> 
> Review of attachment 8732181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's add the comment:
> 
> // Make this available to both AMD and CJS environments
Ah, yes, done.

Honza
Attachment #8732181 - Attachment is obsolete: true
Attachment #8732818 - Flags: review+
Created attachment 8732821 [details] [diff] [review]
bug1256757-json-tree.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> Comment on attachment 8732183 [details] [diff] [review]
> bug1256757-json-tree.patch
> 
> Review of attachment 8732183 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/jsonview/components/json-panel.js
> @@ +6,5 @@
> >  
> >  "use strict";
> >  
> >  define(function(require, exports, module) {
> >    const React = require("devtools/client/shared/vendor/react");
> 
> I think it's more common in other tools to destructure from React, like:
> 
> const { DOM: dom, createFactory } =
> require("devtools/client/shared/vendor/react");
> 
> Probably best for this to match that.  See also the new style guide[1].
> 
> [1]: https://wiki.mozilla.org/DevTools/CodingStandards#React_.26_Redux
True, done.

Honza
Attachment #8732183 - Attachment is obsolete: true
Attachment #8732821 - Flags: review+
Attachment #8732184 - Flags: review?(jryans) → review+
Attachment #8732196 - Flags: review?(jryans) → review+
Thanks Ryan!

Honza
Keywords: checkin-needed

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3be9cf3907ad
https://hg.mozilla.org/mozilla-central/rev/3ddb1c19d100
https://hg.mozilla.org/mozilla-central/rev/f450e16672e4
https://hg.mozilla.org/mozilla-central/rev/3ce5d23d337f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1302708

Updated

2 years ago
Depends on: 1328008

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.