Closed Bug 1244919 Opened 8 years ago Closed 8 years ago

JSON Viewer: show the colon for object attributes

Categories

(DevTools :: JSON Viewer, defect)

defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: clarkbw, Assigned: dalimil)

References

Details

Attachments

(2 files, 1 obsolete file)

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"
Summary: JSON VIewer: show the colon for object attributes → JSON Viewer: show the colon for object attributes
Blocks: 1243951
Attached image updated-treeview.png
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)
Attached patch rev1 (obsolete) — Splinter Review
Here is the above-mentioned patch.
Attachment #8770172 - Flags: review?(clarkbw)
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)
(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 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)
Attached patch rev2Splinter Review
Ok, I added the CSS instead.
Attachment #8770172 - Attachment is obsolete: true
Attachment #8772022 - Flags: review?(odvarko)
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+
Forgot to add, I think it would be worth to push to try to make sure all tests pass.

Honza
I'm relatively new to Mozilla and I don't think I have the permissions to push to the try server...
Flags: needinfo?(odvarko)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/c9c3c840af69
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.