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

Add JSONDataset preview support #1800

Merged
merged 13 commits into from
Mar 28, 2024
Merged

Add JSONDataset preview support #1800

merged 13 commits into from
Mar 28, 2024

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Mar 12, 2024

Description

Following, kedro-org/kedro-plugins#595, we need to add support for previewing JSONDataset in Kedro-Viz.

Development notes

Preview screenshots:

image image

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
@SajidAlamQB SajidAlamQB self-assigned this Mar 12, 2024
@SajidAlamQB SajidAlamQB marked this pull request as draft March 12, 2024 14:26
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
@SajidAlamQB SajidAlamQB marked this pull request as ready for review March 14, 2024 14:37
Signed-off-by: Sajid Alam <[email protected]>
@rashidakanchwala
Copy link
Contributor

Maybe not a part of this ticket but I noticed there's some design inconsistency in the way we show preview for Plotly and Matplotlib and have the Collapse Visualisation icon and for table and json we don't have that. Maybe for @stephkaiser to have a look and see how we can make it more consistent.

Maybe for everything we just have 'Expand Preview' and 'Collapse Preview' ?

Signed-off-by: Sajid Alam <[email protected]>
@ravi-kumar-pilla
Copy link
Contributor

ravi-kumar-pilla commented Mar 25, 2024

Hi @SajidAlamQB ,

Let's say I have a cars.json which is an array of objects and I want to preview first 5 objects in the array and not complete json. Is there a way to do it ? As of now if I use nrows, I receive -

WARNING 'cars' could not be previewed. Full exception: TypeError: preview() got an unexpected keyword argument 'nrows'

cars:
  type: json.JSONDataset
  filepath: ${_base_location}/01_raw/cars.json
  metadata:
    kedro-viz:
      layer: raw
      preview_args:
        nrows: 5

cc: @rashidakanchwala @jitu5

Thank you

@SajidAlamQB
Copy link
Contributor Author

Let's say I have a cars.json which is an array of objects and I want to preview first 5 objects in the array and not complete json. Is there a way to do it ?

@ravi-kumar-pilla not at the moment see here, currently we are just sending the raw json to the fronend as I thought it would be easier for the react-json-view to handle the parsing. Given the range of complex json structures, implementing a generic preview method that can limit the number of rows was quite challenging. Let me know if you think otherwise or have some other ideas.

@ravi-kumar-pilla
Copy link
Contributor

Let's say I have a cars.json which is an array of objects and I want to preview first 5 objects in the array and not complete json. Is there a way to do it ?

@ravi-kumar-pilla not at the moment see here, currently we are just sending the raw json to the fronend as I thought it would be easier for the react-json-view to handle the parsing. Given the range of complex json structures, implementing a generic preview method that can limit the number of rows was quite challenging. Let me know if you think otherwise or have some other ideas.

okay, lets go with showing everything for now then. Thank you

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a release note and this should be good to get merged. Thank you Sajid !!

Signed-off-by: Sajid Alam <[email protected]>
@stephkaiser
Copy link

stephkaiser commented Mar 26, 2024

Maybe not a part of this ticket but I noticed there's some design inconsistency in the way we show preview for Plotly and Matplotlib and have the Collapse Visualisation icon and for table and json we don't have that. Maybe for @stephkaiser to have a look and see how we can make it more consistent.

Maybe for everything we just have 'Expand Preview' and 'Collapse Preview' ?

Thanks Sajid, Rashida and team!

To make the buttons more consistent I suggest we:

  1. Remove the 'Collapse Preview' button for all dataset previews so we only have the Expand button and the Back button (as shown in Sajid's example screenshots above).
  2. Change all Expand button labels to say 'Expand preview'. I think this language should work for all dataset preview types we have: table, json, plotly and matplotlib.

Some additional metadata panel changes for JSON datasets:

  • I also propose we add scrolling functionality in the preview window within the metadata panel for JSON datasets. We can keep the Expand button there as well in case the code is long and the user would like a full view of it.
  • It would also be nice to include this scrolling functionality for table previews. However, since users will most likely need to do both horizontal and vertical scrolling to view the table, only one direction should be allowed at a time, meaning we should not allow users to scroll diagonally using both horizontal and vertical scroll together at the same time. (moved to Metadata panel design fixes #1827)
  • the left padding for JSON code in the preview window should match the padding above for the metadata panel content (48px padding)
  • To minimise the appearance of the wide empty space when the JSON preview is expanded, I've contained it within a code block and added a copy button as well, just like in the Show Code block.
  • Designs can be found here - https://www.figma.com/file/3kSpvIO1veLKfy9qHxuXwF/Kedro-WIP?type=design&node-id=1364-66774&mode=design&t=pIoR3TH0bX1gTk5Y-4

@SajidAlamQB
Copy link
Contributor Author

SajidAlamQB commented Mar 27, 2024

Other metadata panel design fixes: (@rashidakanchwala let me know if I should move this to a separate ticket instead)

I think a separate issue for this would be great, linking this PR in it.

@rashidakanchwala
Copy link
Contributor

Yes, seperate issue would be better. @stephkaiser would u mind raising a ticket on this. thanks :)

@stephkaiser
Copy link

Yes, seperate issue would be better. @stephkaiser would u mind raising a ticket on this. thanks :)

done! :) #1827

@SajidAlamQB SajidAlamQB merged commit 31caffe into main Mar 28, 2024
5 checks passed
@SajidAlamQB SajidAlamQB deleted the add-json-preview-support branch March 28, 2024 12:34
@jitu5 jitu5 mentioned this pull request Apr 16, 2024
5 tasks
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.

5 participants