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

feat: implement JsonViewer widget #479

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Conversation

madeindjs
Copy link
Collaborator

@madeindjs madeindjs commented Jun 27, 2024

Implement a brand new CoreJsonViewer component

Screencast.from.2024-07-09.21-28-15.webm

This component uses new base components as:

  • BaseCollapsible, a generic HTML <details>/<summary> which can be reused later
  • BaseJsonViewer*, some interns components to build CoreJsonViewer

@madeindjs madeindjs force-pushed the jsonViewver branch 2 times, most recently from 875f80a to 28db80f Compare June 27, 2024 08:21
@FabienArcellier
Copy link
Collaborator

I feel like I would want to open level 2 or level 3 easily without giving complex jsonpath expression.

@ramedina86 @madeindjs Would it be interesting to have a complementary Opened level field with Paths open ?

Opened level: 0 # none (default)
Opened level: -1 # all
Opened level: 1 # open all level one
name: Opened level
desc: Open a specific level of json object (-1 to open all)

@ramedina86
Copy link
Collaborator

I feel like I would want to open level 2 or level 3 easily without giving complex jsonpath expression.

@ramedina86 @madeindjs Would it be interesting to have a complementary Opened level field with Paths open ?

Opened level: 0 # none (default)
Opened level: -1 # all
Opened level: 1 # open all level one
name: Opened level
desc: Open a specific level of json object (-1 to open all)

@FabienArcellier Yes I wasn't convinced that we needed paths in the first place but @madeindjs thought we should keep it

I'm not in favor of keeping both but I'd prefer Fabien's approach one over the current path mechanism, unless @madeindjs can convince me with a solid use case.

@FabienArcellier
Copy link
Collaborator

I think a copy action would be really helpful for json. Does it make sense ?

image

@FabienArcellier
Copy link
Collaborator

I'd prefer Fabien's approach one over the current path mechanism, unless @madeindjs can convince me with a solid use case.

I am convinced it's interesting to keep it. I see a usecase for implementing search in huge json file. It allow advanced scenario for people.

@ramedina86
Copy link
Collaborator

Ok let's keep the paths.

I think a copy action would be really helpful for json. Does it make sense ?

That'd be great, but let's do that at a different stage. @polymorpheuz is working on a bar to enable "Copy" in Annotated Text component, so once we merge that we can reuse and apply to this one. I'll create a ticket.

@madeindjs
Copy link
Collaborator Author

Thanks for your feedback @ramedina86 & @FabienArcellier !

About the pathsOpen, yes, my argument was actually to "highlights" a part of a JSON without expanding everything (as @FabienArcellier pointed it). For instance, in the example below, user might want to display foo.bar without showing others

{
  "foo": { "bar": "value" },
  "others": [/* 1K items */]
}

But,I don't know well the product to know if it's a solid use case.

Also, little disclaimer, my implementation of the JSON path is a bit naive. It doesn't work for keys containing . like { "foo.bar.1": "value" }

return fields.pathsOpen.value.map((pathStr) => String(pathStr).split("."));

I'm fine to implement @FabienArcellier 's idea or to not expose this setting for now.

@ramedina86
Copy link
Collaborator

Good point about the dots, what we could do is use arrays

["foo", "bar", "1"]

@FabienArcellier what do you think?

@FabienArcellier
Copy link
Collaborator

FabienArcellier commented Jun 27, 2024

If we don't follow Jsonpath or JMESPath (two json query standard), I would advocate we don't propose the feature yet. It would be too hard to explain and document for the user.

If we want to propose this feature, I would advocate to use external library as jsonpath

@ramedina86
Copy link
Collaborator

@madeindjs ok let's drop the open specific paths for now and go for @FabienArcellier's idea (as list of options)

Opened level: 0 # none (default)
Opened level: -1 # all
Opened level: 1 # open all level one

@FabienArcellier
Copy link
Collaborator

FabienArcellier commented Jul 7, 2024

It's almost looks perfect. I enjoy the component. I was surprised when I set 0, the level 1 is opened. Is it voluntary ?

image

@FabienArcellier
Copy link
Collaborator

Decision :

  • collapse everything when 0 is set as initial depth
  • rename open level as initial depth

@madeindjs
Copy link
Collaborator Author

@ramedina86 & @FabienArcellier , I implemented the final modifications

Screencast.from.2024-07-09.21-28-15.webm

@ramedina86 ramedina86 merged commit 9c4730b into writer:dev Jul 23, 2024
16 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants