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

Feature/url parameters #1138

Merged
merged 64 commits into from
Nov 1, 2022
Merged

Feature/url parameters #1138

merged 64 commits into from
Nov 1, 2022

Conversation

Huongg
Copy link
Contributor

@Huongg Huongg commented Oct 19, 2022

Description

Fixes #1117

I use Query parameters rather than URI parameters, hence all of them are combined into a single string as below

main: '/',
focusedNode: `/?${pipelineActive}&${params.focused}:id`,
selectedNode: `/?${pipelineActive}&${params.selected}:id`,

As discussed with the engineer and design team, the URL parameter should cover a few scenarios, such as

  • when selecting/clicking on a single node , the URL will includeselected_id:...
  • it also recognises whether the node belongs to a modular pipeline or not, if it does then expand the modular pipeline list
  • when focusing on a modular pipeline the URL will include focused_id:...
  • we don't update the parameter when switching between different pipeline views but we do include the view in the URL parameter when the user performs an action. eg ?pipeline_id=Modelling%20stage&selected_id=cae2d1c7
  • it also works when you click on the flowchart

Development notes

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

Huongg added 20 commits October 17, 2022 15:41
Signed-off-by: huongg <[email protected]>
@Huongg Huongg requested review from comym and Mackay031 October 19, 2022 16:55
@tynandebold
Copy link
Member

That error message is looking really good! Well done.

Why does the page crash with an incorrect pipeline ID but not an incorrect node ID? In any event, I think how you handled that before in your previous linked commit can work. The main thing is that the app doesn't crash like in your third screenshot above.

@Huongg
Copy link
Contributor Author

Huongg commented Oct 27, 2022

That error message is looking really good! Well done.

Why does the page crash with an incorrect pipeline ID but not an incorrect node ID? In any event, I think how you handled that before in your previous linked commit can work. The main thing is that the app doesn't crash like in your third screenshot above.

hey sounds good, I've gone back to how I did it before, the error messages should now cover all cases, incl invalid pipeline ID

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.

Amazing! Approved. Great work on this one, Huong. Thanks so much for seeing it through.

I have one worry that some of the URL params we're adding may potentially mess with some of the jupyter-server-proxy stuff @AntonyMilneQB worked on that I helped him with, though it's hard to tell right now. I don't remember how to test that at the moment, so I think for now it'll have to be ok!

<div className="kedro-pipeline">
<Sidebar />
<MetaData />
<PipelineWarning errorMessage={errorMessage} invalidUrl={invalidUrl} />
Copy link
Member

Choose a reason for hiding this comment

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

Is it too much of a pain to pass these props to the same component on line 66? I guess there might be some other work to do it you put it there and don't have this if / else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey the reason I separate them with if/else statements so it won't render the Flowchart + any other things, cause other wise the flowchart will still show up in the background

@Huongg Huongg requested a review from yetudada as a code owner October 28, 2022 09:20
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.

Hi @Huongg --- looks' greatt!! <3

i just came across another error handling issue, when we type something incorrect in the focus mode then it's a blank screen.

try http://localhost:4141/?pipeline_id=__default__&focused_id=featured_engineering instead of http://localhost:4141/?pipeline_id=__default__&focused_id=feature_engineering

@Huongg
Copy link
Contributor Author

Huongg commented Oct 28, 2022

Hi @Huongg --- looks' greatt!! <3

i just came across another error handling issue, when we type something incorrect in the focus mode then it's a blank screen.

try http://localhost:4141/?pipeline_id=__default__&focused_id=featured_engineering instead of http://localhost:4141/?pipeline_id=__default__&focused_id=feature_engineering

hey @rashidakanchwala sorry i didnt push my latest changes to it, it should be fixed now with the invalid Modular Pipeline

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.

Thanks @Huongg .. super excited abt this feature release :)

@Huongg
Copy link
Contributor Author

Huongg commented Nov 1, 2022

I've updated the copy for the error message suggested by @stichbury. @comym @Mackay031 if you're happy can i get approval for this PR please?

Screenshot 2022-11-01 at 11 45 42

Signed-off-by: huongg <[email protected]>
@Huongg Huongg merged commit dca94f0 into main Nov 1, 2022
@Huongg Huongg deleted the feature/URL-parameters-1 branch November 1, 2022 14:08
@tynandebold tynandebold mentioned this pull request Jan 18, 2023
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.

Create URL parameters for each element/section in kedro-viz
4 participants