Closed Bug 1256757 Opened 7 years ago Closed 7 years ago

JSON Viewer: use shared tree component

Categories

(DevTools :: JSON Viewer, defect, P2)

Product:
DevTools
Component:
JSON Viewer
Type:
defect

Tracking

(firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(4 files, 2 obsolete files)

bug1256757-expose-common-styles.patch
2.82 KB, patch
jryans
: review+
Details | Diff | Splinter Review
bug1256757-tests.patch
4.98 KB, patch
jryans
: review+
Details | Diff | Splinter Review
bug1256757-tree-amd.patch
49.79 KB, patch
Honza
: review+
Details | Diff | Splinter Review
bug1256757-json-tree.patch
37.71 KB, patch
Honza
: review+
Details | Diff | Splinter Review 
JSON Viewer should reuse shared tree component from:
devtools/client/shared/components/tree

Honza
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Attached patch bug1256757-tree-amd.patch (obsolete) — — Splinter Review 
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)
Attached patch bug1256757-json-tree.patch (obsolete) — — Splinter Review 
Using shared tree from: devtools/shared/components/tree and removing the old one.

Honza
Attachment #8732183 - Flags: review?(jryans)
Exposing common.css through resource:// protocol (just like variables.css) and using it in JSON Viewer.

Honza
Attachment #8732184 - Flags: review?(jryans)
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+
(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+
(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+
Thanks Ryan!

Honza
Keywords: checkin-needed
Depends on: 1302708
Depends on: 1328008
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.