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.

Sign up for GitHub

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

Jump to bottom

Kedro Viz Lite User Warning UI #2092

Merged
merged 27 commits into from
Sep 17, 2024
Merged

Kedro Viz Lite User Warning UI #2092

merged 27 commits into from
Sep 17, 2024

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Sep 11, 2024 •
edited
Loading

Description

Resolves #2058

Development notes

Backend changes:

  • Refactored PackageCompatibilityAPI response to MetadataAPIResponse
  • Updated the API endpoint from [GET] "/package-compatibilities" to "/metadata" and updated the response model
  • Movedget_package_compatibilities to utils
  • Created models to hold PackageCompatibility and Metadata for the application

Frontend changes:

  • Updated function calls to fetchMetadata as per the backend updated endpoint
  • Added showBanner to the redux store and created generic actions, reducers to extend banner functionality in future
  • Created Banner components and added styles
  • Added tests for banner component

Thank you @jitu5 for helping out in resolving the redux states 💯

image

image

QA notes

  • All tests should pass
  • You should see the banner when there are missing dependencies in your python env and you do kedro viz --lite. The banner should not appear again for the rest of the browser session once you close it.
  • The Learn More button click should take you to the docs for --lite
  • There should not be any changes with the package_compatibility feature we have in deploy and ET.

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

@ravi-kumar-pilla
sync remote
8830cab
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
0a5d1ae
@ravi-kumar-pilla
merge main
bf47b47
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
13e905b
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
3d6326c
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
3d3bdda
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
1abfc43
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
1461a3c
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz
c3f7cc6
@ravi-kumar-pilla
add lite viz banner
61a871b
@ravi-kumar-pilla
banner design change
3cc318e
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…
39a5b45 
…ature/lite-user-warning-ui
@ravi-kumar-pilla
add tests
54f18a1
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…
2191529 
…ature/lite-user-warning-ui
@ravi-kumar-pilla
refactor metadata api
497d03c
@ravi-kumar-pilla
fix permissions
17c85a8
@ravi-kumar-pilla
working state
d4bf0d2
@ravi-kumar-pilla
comment out banner condition for testing
3495f52
@ravi-kumar-pilla
Merge branch 'main' into feature/lite-user-warning-ui
26d8cfb
@stephkaiser
Copy link

thanks Ravi, that was super quick!

some minor design QA comments from me:

  • font size for the title and description should be 16, looks like its currently 14
  • height of the banner should be 68px max, looks like its 77px currently
Screenshot 2024-09-13 at 17 50 14

thank you!

@ravi-kumar-pilla ravi-kumar-pilla changed the title Feature/lite user warning UI Kedro Viz Lite User Warning UI Sep 13, 2024
@ravi-kumar-pilla ravi-kumar-pilla marked this pull request as ready for review September 17, 2024 04:16
Huongg
Huongg reviewed Sep 17, 2024
View reviewed changes
src/config.js
@@ -5,6 +5,7 @@ export const localStorageFlowchartLink = 'KedroViz-link-to-flowchart';
export const localStorageMetricsSelect = 'KedroViz-metrics-chart-select';
export const localStorageRunsMetadata = 'KedroViz-runs-metadata';
export const localStorageShareableUrl = 'KedroViz-shareable-url';
export const localStorageBannerStatus = 'KedroViz-banners';
Copy link
Contributor

Choose a reason for hiding this comment

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

hey what does this localStorage do? Is it to track if user has seen the banner before? If so? I've done something similar too for feedback component, perhaps we can have a group call "localStoragePopup" something like that, and it can be shared between banner and feedback?

Copy link
Contributor

Choose a reason for hiding this comment

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

i like that. maybe we can refactor localStorage in a future ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we can have a common key in future tickets

src/styles/_variables.scss
@@ -31,6 +31,7 @@ $run-list-controls-height: 95px;
$z-index-metadata-panel: 3;
$z-index-metadata-code: 3;
$z-index-MuiTreeItem-icon: 1;
$z-index-banner: 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

6 is a very high number, is there something underneath it that has z-index of 5? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes there is global toolbar and feature hints with z-index:5 . Another reason I thought is we should always have our banners on top of other components, so made it a 6.

@Huongg
Copy link
Contributor

Huongg commented Sep 17, 2024

this is pretty cool, thanks @ravi-kumar-pilla. Just a quick question about the setup, I'm trying to run it locally, but i have the error Error: No such option: --lite Did you mean --pipeline? when i ran kedro viz --lite

I've tested with your branch with a new environment. Is there anything else I need to do?

Screenshot 2024-09-17 at 11 15 31

rashidakanchwala
rashidakanchwala approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It works well. and I have looked through the code quickly. There are some possible refactorings we can focus on FE (not caused by this PR but previous ones as well) in the future which we can look into after this release. Thanks a lott @ravi-kumar-pilla , passing data through api/metadata looks great

Huongg
Huongg approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

its working now for me, thanks @ravi-kumar-pilla

@ravi-kumar-pilla ravi-kumar-pilla merged commit 30d5cef into main Sep 17, 2024
40 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the feature/lite-user-warning-ui branch September 17, 2024 16:12
@ravi-kumar-pilla ravi-kumar-pilla mentioned this pull request Sep 17, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rashidakanchwala rashidakanchwala rashidakanchwala approved these changes

@Huongg Huongg Huongg approved these changes

@stephkaiser stephkaiser stephkaiser approved these changes

@jitu5 jitu5 Awaiting requested review from jitu5 jitu5 is a code owner

@astrojuanlu astrojuanlu Awaiting requested review from astrojuanlu astrojuanlu is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kedro Viz Lite User Warnings
4 participants
@ravi-kumar-pilla @stephkaiser @Huongg @rashidakanchwala