Closed
Bug 1244919
Opened 8 years ago
Closed 8 years ago
JSON Viewer: show the colon for object attributes
Categories
(DevTools :: JSON Viewer, defect)
DevTools
JSON Viewer
Tracking
(firefox50 fixed)
RESOLVED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: clarkbw, Assigned: dalimil)
References
Details
Attachments
(2 files, 1 obsolete file)
171.44 KB,
image/png
|
Details | |
802 bytes,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
Because we are showing the colon in the object summary we do make this inconsistent and I think it will be better with the colons back. via bug 1223143 comment 7 > ## JSON tab - readability - too much punctuation removed? > > Except in the object summaries, this punctuation is removed: > > - commas > - colons > - curly brackets (for expanded objects) > - square brackets (for expanded arrays) > > I understand doing away with the commas, and relying on new lines only. But > I would reinstate the colons to not be too far from JSON syntax (which users > already know). Right now when users are faced with: > > some_key "hello…goodbye" > other 4 > > it can be a bit hard to parse, and the colons (or another visual separator, > but why invent a new one unrelated to the actual syntax?) would help: > > some_key: "hello…goodbye" > other: 4 > > For the brackets, I find it puzzling to have them in the object and array > summaries but not having them wrap the expanded data. This does not convey > "full content of an object" to me: > > some_key { annie: "are you okay", are_you_okay: "annie" } > annie "are you okay" > are_you_okay "annie" > > Similarly for an array (though the lack of repetition makes it not as bad): > > some_key [4] > 0 "annie" > 1 "are you okay" > 2 "are you okay" > 4 "annie"
Reporter | ||
Updated•8 years ago
|
Summary: JSON VIewer: show the colon for object attributes → JSON Viewer: show the colon for object attributes
Assignee | ||
Comment 1•8 years ago
|
||
I made a patch for this, but I have a few doubts... Jsonview uses the shared tree-view components and since I am new to Mozilla, I don't really know which other modules might be affected when I change this file (- grep suggests 'webconsole', 'dom', ...) Also, the semicolon has the same pink colour as the attribute name - not sure if this is desired...? See the screenshot...
Assignee: nobody → dalimilhajek
Status: NEW → ASSIGNED
Flags: needinfo?(clarkbw)
Assignee | ||
Comment 2•8 years ago
|
||
Here is the above-mentioned patch.
Attachment #8770172 -
Flags: review?(clarkbw)
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8770172 [details] [diff] [review] rev1 Good questions. Would a CSS selector + content: ":" be possible? Bumping review over to Honza who could answer them.
Flags: needinfo?(clarkbw)
Attachment #8770172 -
Flags: review?(clarkbw) → review?(odvarko)
Comment 4•8 years ago
|
||
(In reply to Dalimil Hajek [:dalimil] from comment #1) > I made a patch for this, but I have a few doubts... Jsonview uses the shared > tree-view components and since I am new to Mozilla, I don't really know > which other modules might be affected when I change this file (- grep > suggests 'webconsole', 'dom', ...) Yes, this change will affect: - JSON View - DOM panel - HTTPi in the Console panel But, I am okay with the change. > Also, the semicolon has the same pink color as the attribute name - not > sure if this is desired...? Should use: var(--object-color); If this is fixed I can review. Honza
Comment 5•8 years ago
|
||
Comment on attachment 8770172 [details] [diff] [review] rev1 Review of attachment 8770172 [details] [diff] [review]: ----------------------------------------------------------------- Removing the review flag, see my comment #4
Attachment #8770172 -
Flags: review?(odvarko)
Assignee | ||
Comment 6•8 years ago
|
||
Ok, I added the CSS instead.
Attachment #8770172 -
Attachment is obsolete: true
Attachment #8772022 -
Flags: review?(odvarko)
Comment 7•8 years ago
|
||
Comment on attachment 8772022 [details] [diff] [review] rev2 Review of attachment 8772022 [details] [diff] [review]: ----------------------------------------------------------------- Works for me, thanks! Honza
Attachment #8772022 -
Flags: review?(odvarko) → review+
Comment 8•8 years ago
|
||
Forgot to add, I think it would be worth to push to try to make sure all tests pass. Honza
Assignee | ||
Comment 9•8 years ago
|
||
I'm relatively new to Mozilla and I don't think I have the permissions to push to the try server...
Flags: needinfo?(odvarko)
Comment 10•8 years ago
|
||
I see, done: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44ffa70475e2 Honza
Flags: needinfo?(odvarko)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/c9c3c840af69 JSON Viewer: show the colon for object attributes. r=odvarko
Keywords: checkin-needed
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9c3c840af69
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•