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

Fix- Flowchart does not load for static data inputs #843

Merged
merged 21 commits into from
May 5, 2022

Conversation

studioswong
Copy link
Contributor

@studioswong studioswong commented May 5, 2022

Description

fixes issue 840.

Currently the flowchart would not load for static data soures, which causes problems for our npm component usecase. This PR fixes that.

Development notes

This problem is mainly due to the fact that the getGraphInput action does not get dispatched on first load ( it is only called via web worker which will only be triggered for future state changes. ) I have added an action within the configureStore method to ensure this action still gets fired on first load.

I have also modified the test set up with mocking the configureStore function to ensure it properly configures the store.

Please note that I had not added any tests for this given the lack of a test suite setup for store.js, and it might be an overkill to add that just for testing the dispatch of this action( especially this is a quick fix with the aim to be included in the release tomorrow).

That said, happy to look into adding one for store.js in general in a future ticket.

QA notes

Try loading the app via one of the static data sources - we should now be able to load the flowchart properly.

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

@studioswong studioswong requested a review from yetudada as a code owner May 5, 2022 09:03
@tynandebold tynandebold linked an issue May 5, 2022 that may be closed by this pull request
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

This is great and is working as expected, though I think your added if condition is always true. Looks like you need to pass a second argument into the configureStore function here so the app knows if it should calculate the graph when the store is being configured.

this.store = configureStore(initialState, initialState.dataSource);

src/store/store.js Show resolved Hide resolved
src/store/store.js Show resolved Hide resolved
@studioswong
Copy link
Contributor Author

This is great and is working as expected, though I think your added if condition is always true. Looks like you need to pass a second argument into the configureStore function here so the app knows if it should calculate the graph when the store is being configured.

this.store = configureStore(initialState, initialState.dataSource);

good catch, turns out I forgot to commit this change - I've just pushed it in.

The change would be this.store = configureStore(initialState, this.props.data);

@studioswong studioswong requested a review from tynandebold May 5, 2022 10:55
RELEASE.md Outdated Show resolved Hide resolved
@studioswong studioswong requested a review from tynandebold May 5, 2022 10:59
RELEASE.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM. thanks @studioswong

@rashidakanchwala
Copy link
Contributor

we have this same issue with running kedro-viz on jupyter notebooks and databricks. the first time -- it displays a blank viz and then when u click on something, the viz loads. i am hoping this fix will sort that out too... magically ;)

@studioswong
Copy link
Contributor Author

we have this same issue with running kedro-viz on jupyter notebooks and databricks. the first time -- it displays a blank viz and then when u click on something, the viz loads. i am hoping this fix will sort that out too... magically ;)

interesting - that sounds like it could be likely a related issue - I imagine both uses cases are passing a json object directly as a prop instead of retrieving data from the server?

Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Working great

@studioswong studioswong merged commit fd9a32e into main May 5, 2022
@studioswong studioswong deleted the fix/fix-viz-npm-loading-issue branch May 5, 2022 15:40
@studioswong studioswong mentioned this pull request May 5, 2022
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.

Fix: Flowchart does not load for JSON data inputs
3 participants