Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only display node structure in JSONTree for arrays and empty objects #7227

Merged
merged 3 commits into from Sep 25, 2019
Merged

Only display node structure in JSONTree for arrays and empty objects #7227

merged 3 commits into from Sep 25, 2019

Conversation

tibdex
Copy link
Member

@tibdex tibdex commented Sep 18, 2019

I feel like the item strings draw the attention away from the actual content of the tree.

Code changes

Pass an additional getItemString prop to <JSONTree />.

User-facing changes

In a notebook

Before

Screenshot_2019-09-18 JupyterLab(2)

After

Screenshot_2019-09-18 JupyterLab(1)

Exploring a JSON file

Before

Screenshot_2019-09-18 JupyterLab(3)

After

Screenshot_2019-09-18 JupyterLab


If you feel this is too much, we could do like Google Chrome DevTools console which only shows this information for arrays (not for objects):

Screenshot 2019-09-18 at 17 43 58

Backwards-incompatible changes

None.

The item strings draw the attention of the user away from the actual content of the tree.
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Sep 18, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@vidartf
Copy link
Member

@vidartf vidartf commented Sep 18, 2019

In the screenshot, it seems types (at the bottom) is no longer showing the empty array (it is just blank), so that would probably need to be fixed.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 18, 2019

What does it show after this change for non-empty arrays and for empty objects?

{"emptyobject": {}, "emptyarray": [], "fullarray": [1,2,3,4,5], "nested array": [{}, {"a": 1, "b": 2}, []]}

@tibdex
Copy link
Member Author

@tibdex tibdex commented Sep 18, 2019

In the screenshot, it seems types (at the bottom) is no longer showing the empty array (it is just blank), so that would probably need to be fixed.

It was already blank before but the itemType (in that case [] 0 items) helped realizing that.

We could display the itemType only when the collection is empty. Here is how it looks:

Screenshot_2019-09-18 JupyterLab(6)

@telamonian
Copy link
Member

@telamonian telamonian commented Sep 18, 2019

I'd lean towards leaving in the count of array items. It's certainly much more useful information than the object key count in most circumstances. But either way is fine

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 18, 2019

We could display the itemType only when the collection is empty. Here is how it looks:

+1. That looks a lot better

I'd lean towards leaving in the count of array items. It's certainly much more useful information than the object key count in most circumstances.

I would also lean towards having the array count displayed, but I also agree it's not a huge deal to me right now.

@tibdex tibdex changed the title Don't display node structure in json-extension Only display node structure in JSONTree for arrays and empty objects Sep 18, 2019
@tibdex
Copy link
Member Author

@tibdex tibdex commented Sep 18, 2019

Alright, with array details always displayed and object details only displayed when they are empty:

Screenshot 2019-09-18 at 22 09 28

If you're all good with that, you can squash and merge ;)

@jasongrout jasongrout added this to the 1.2 milestone Sep 18, 2019
Copy link
Contributor

@jasongrout jasongrout left a comment

Thanks!

@tibdex
Copy link
Member Author

@tibdex tibdex commented Sep 25, 2019

Some checks failed but it doesn't seem related to this PR's changes. Is there something I can do to help get this merged?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 25, 2019

For others wanting to test this, here is the code above for convenience:

from dataclasses import dataclass

@dataclass
class Tree:
    data: dict
    def _repr_json_(self):
        return self.data, {"expanded": True}
Tree({"emptyobject": {}, "emptyarray": [], "fullarray": [1,2,3,4,5], "nested array": [{}, {"a": 1, "b": 2}, []]})

@jasongrout jasongrout merged commit 12821a3 into jupyterlab:master Sep 25, 2019
7 of 9 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 25, 2019

@meeseeksdev backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this issue Sep 25, 2019
jasongrout added a commit that referenced this issue Sep 25, 2019
…7-on-1.x

Backport PR #7227 on branch 1.x (Only display node structure in JSONTree for arrays and empty objects)
@jasongrout jasongrout removed this from the 1.2 milestone Sep 25, 2019
@jasongrout jasongrout added this to the 2.0 milestone Sep 25, 2019
@lock lock bot added the status:resolved-locked label Oct 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants