Closed jitu5 closed 5 months ago
A minor inconsistency during Reset View. As mentioned in the ticket that the default qParams are - pid, types and expandAllPipelines. But when I click on Reset View, the pid is not present in the URL as shown below, but it does appear back if I open a new window.
Also, I am not sure of the feasibility part but can we have the query params follow some order while we append to the URL ?
Thank you
A minor inconsistency during Reset View. As mentioned in the ticket that the default qParams are - pid, types and expandAllPipelines. But when I click on Reset View, the pid is not present in the URL as shown below, but it does appear back if I open a new window.
Also, I am not sure of the feasibility part but can we have the query params follow some order while we append to the URL ?
Thank you
I changed to retain pid after click on reset view. for query params to follow some order, let me check the feasibility.
great work @jitu5, after a look through my first notice is
tags=...
doesnt get update immediately unless i refresh the page. It might actually a different bug too as if you notice, once you filtered the tag and refresh the page, the previous tags that you filtered are resetScreen.Recording.2024-03-21.at.09.10.25.mov
@Huongg for both the cases updating tags in url and tags gets reset on refresh works well at my end.
https://github.com/kedro-org/kedro-viz/assets/38945204/63d2293c-12e6-4d33-a493-b6b2b0173c3e
Hi @jitu5 ,
There seems to be an issue with tag filtering (this is present in demo site as well, so it might be a bug) but it would be nice to resolve it here.
Steps:
Hi @jitu5 ,
There seems to be an issue with tag filtering (this is present in demo site as well, so it might be a bug) but it would be nice to resolve it here.
Steps:
- Start viz
- Click on tag filter Companies. This will show both Companies and Ingestion nodes
- Refresh the page and the flowchart appears to only have Companies node.
- I tried similar filtering with other tags and it behaves differently.
@ravi-kumar-pilla @rashidakanchwala
Here I see two diffreant behaviour when expandAllPipelines
is true and false.
When expandAllPipelines
is false:
https://github.com/kedro-org/kedro-viz/assets/38945204/8895f5fb-3b5a-41fe-9868-d081903634ee
When expandAllPipelines
is true:
https://github.com/kedro-org/kedro-viz/assets/38945204/2ee1e3dc-cb9e-4fa7-87c6-f3fa3f3b4729
This is my understanding not completely sure is this a right behaviour.
Hi @jitu5 , There seems to be an issue with tag filtering (this is present in demo site as well, so it might be a bug) but it would be nice to resolve it here. Steps:
- Start viz
- Click on tag filter Companies. This will show both Companies and Ingestion nodes
- Refresh the page and the flowchart appears to only have Companies node.
- I tried similar filtering with other tags and it behaves differently.
@ravi-kumar-pilla @rashidakanchwala Here I see two diffreant behaviour when
expandAllPipelines
is true and false.When
expandAllPipelines
is false:
Start viz
Click on tag filter Companies. This will show both Companies and Ingestion nodes(not its child node which contains Companies tag because Ingestion nodes collapsed on left side)
Refresh the page and the flowchart appears to only have Companies node.
Screen.Recording.2024-03-22.at.10.21.26.a.m.mov When
expandAllPipelines
is true:Start viz
Click on tag filter Companies. This will show both Companies, Apply Types To Companies & Int Typed Companies which contains Companies tag.
Refresh the page and the flowchart appears same as before refresh.
Screen.Recording.2024-03-22.at.10.17.06.a.m.mov This is my understanding not completely sure is this a right behaviour.
There seems to be some bug/inconsistency here. I am not sure if that is stemming from the backend response or filtering happening on the frontend. Let me take some time and debug the issue. I feel the output should honor all the filtering criteria (even for the refresh) i.e.,
Thank you
Hi @jitu5 , There seems to be an issue with tag filtering (this is present in demo site as well, so it might be a bug) but it would be nice to resolve it here. Steps:
- Start viz
- Click on tag filter Companies. This will show both Companies and Ingestion nodes
- Refresh the page and the flowchart appears to only have Companies node.
- I tried similar filtering with other tags and it behaves differently.
@ravi-kumar-pilla @rashidakanchwala Here I see two diffreant behaviour when
expandAllPipelines
is true and false. WhenexpandAllPipelines
is false:
- Start viz
- Click on tag filter Companies. This will show both Companies and Ingestion nodes(not its child node which contains Companies tag because Ingestion nodes collapsed on left side)
- Refresh the page and the flowchart appears to only have Companies node.
Screen.Recording.2024-03-22.at.10.21.26.a.m.mov When
expandAllPipelines
is true:
- Start viz
- Click on tag filter Companies. This will show both Companies, Apply Types To Companies & Int Typed Companies which contains Companies tag.
- Refresh the page and the flowchart appears same as before refresh.
Screen.Recording.2024-03-22.at.10.17.06.a.m.mov This is my understanding not completely sure is this a right behaviour.
There seems to be some bug/inconsistency here. I am not sure if that is stemming from the backend response or filtering happening on the frontend. Let me take some time and debug the issue. I feel the output should honor all the filtering criteria (even for the refresh) i.e.,
- If there is a tag filter, it should show all the nodes containing the tag.
- Incase of expandModularPipelines = false and the filtered nodes happened to be inside a modular pipeline, it should show the modular pipeline node (collapsed view) which seems to be missing now on refresh.
- In case of expandModularPipelines = true and the filtered nodes happened to be inside a modular pipeline, it should show the expanded view (as it is happening now)
Thank you
I tried debugging the issue but it seems to be missing some part of the logic in the frontend in handling the below case -
Incase of expandModularPipelines = false and the filtered nodes happened to be inside a modular pipeline, it should show the modular pipeline node (collapsed view) which seems to be missing now on refresh.
The bug is not completely related to the current feature but it would be good to resolve it here. If the team feels to tackle this in a separate ticket, I am happy to create one. Please let me know what you think of this @rashidakanchwala @jitu5
Hi @jitu5 , There seems to be an issue with tag filtering (this is present in demo site as well, so it might be a bug) but it would be nice to resolve it here. Steps:
- Start viz
- Click on tag filter Companies. This will show both Companies and Ingestion nodes
- Refresh the page and the flowchart appears to only have Companies node.
- I tried similar filtering with other tags and it behaves differently.
@ravi-kumar-pilla @rashidakanchwala Here I see two diffreant behaviour when
expandAllPipelines
is true and false. WhenexpandAllPipelines
is false:
- Start viz
- Click on tag filter Companies. This will show both Companies and Ingestion nodes(not its child node which contains Companies tag because Ingestion nodes collapsed on left side)
- Refresh the page and the flowchart appears to only have Companies node.
Screen.Recording.2024-03-22.at.10.21.26.a.m.mov When
expandAllPipelines
is true:
- Start viz
- Click on tag filter Companies. This will show both Companies, Apply Types To Companies & Int Typed Companies which contains Companies tag.
- Refresh the page and the flowchart appears same as before refresh.
Screen.Recording.2024-03-22.at.10.17.06.a.m.mov This is my understanding not completely sure is this a right behaviour.
There seems to be some bug/inconsistency here. I am not sure if that is stemming from the backend response or filtering happening on the frontend. Let me take some time and debug the issue. I feel the output should honor all the filtering criteria (even for the refresh) i.e.,
- If there is a tag filter, it should show all the nodes containing the tag.
- Incase of expandModularPipelines = false and the filtered nodes happened to be inside a modular pipeline, it should show the modular pipeline node (collapsed view) which seems to be missing now on refresh.
- In case of expandModularPipelines = true and the filtered nodes happened to be inside a modular pipeline, it should show the expanded view (as it is happening now)
Thank you
I tried debugging the issue but it seems to be missing some part of the logic in the frontend in handling the below case -
Incase of expandModularPipelines = false and the filtered nodes happened to be inside a modular pipeline, it should show the modular pipeline node (collapsed view) which seems to be missing now on refresh.
The bug is not completely related to the current feature but it would be good to resolve it here. If the team feels to tackle this in a separate ticket, I am happy to create one. Please let me know what you think of this @rashidakanchwala @jitu5
Thanks for flagging this, I had attempted to address this issue in the frontend (#1542) -- but I reckon it's not the best way. After some investigation, I think we need to fix this issue in the backend. Let's create a new issue.
Hi @jitu5 , Nice work.
I am trying out the scenario below -
Lets say someone would like to share a URL - http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false
. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ?
Thank you
Hi @jitu5 , Nice work.
I am trying out the scenario below - Lets say someone would like to share a URL -
http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false
. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ?Thank you
@ravi-kumar-pilla if you see the table above for nodeType with new browser & empty url(no nodeType params), the default ( types=task,data ) will apply. this is desired. If user wanted to to share with all nodeTypes, all should be included in urls.
Hi @jitu5 , Nice work. I am trying out the scenario below - Lets say someone would like to share a URL -
http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false
. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ? Thank you@ravi-kumar-pilla if you see the table above for nodeType with new browser & empty url(no nodeType params), the default ( types=task,data ) will apply. this is desired. If user wanted to to share with all nodeTypes, all should be included in urls.
okay got it. I think it is a bit of design question as well. Should we make allSelected nodeTypes == noneSelected nodeTypes ? That is how the UI behaves at the moment. What do you think ? @stephkaiser @jitu5 @rashidakanchwala
Hi @jitu5 , Appreciate your patience 💯
I am testing the below scenario -
Please let me know if you need more information. Thank you
Hi @jitu5 , Appreciate your patience 💯
I am testing the below scenario -
- I have a tag filter with companies selected
- I go to incognito and paste the url - it works fine
- I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
- I tested the scenario on normal browser and it has the same issue
Please let me know if you need more information. Thank you
@rashidakanchwala if see below table
this is expected behaviour.
Hi @jitu5 , Nice work. I am trying out the scenario below - Lets say someone would like to share a URL -
http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false
. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ? Thank you@ravi-kumar-pilla if you see the table above for nodeType with new browser & empty url(no nodeType params), the default ( types=task,data ) will apply. this is desired. If user wanted to to share with all nodeTypes, all should be included in urls.
okay got it. I think it is a bit of design question as well. Should we make allSelected nodeTypes == noneSelected nodeTypes ? That is how the UI behaves at the moment. What do you think ? @stephkaiser @jitu5 @rashidakanchwala
Thanks Ravi, do you mind rephrasing or explaining in a less-technical way? Im struggling to follow :) are we discussing if the way all/none node types are displayed in the URL is intuitive to users?
I also had a question regarding the types - what are the task and data types? From the UI i see we have Nodes, Datasets, or Parameters so i'm a bit confused.
Another question - shouldn't all pipelines be expanded by default?
Hi @jitu5 , Appreciate your patience 💯
I am testing the below scenario -
- I have a tag filter with companies selected
- I go to incognito and paste the url - it works fine
- I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
- I tested the scenario on normal browser and it has the same issue
Please let me know if you need more information. Thank you
it does seem a bit strange that users wont be able to tweak the URL themselves. What happens if the user clicks on the Companies tag to remove it from the UI, will the URL update?
Hi @jitu5 , Appreciate your patience 💯 I am testing the below scenario -
- I have a tag filter with companies selected
- I go to incognito and paste the url - it works fine
- I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
- I tested the scenario on normal browser and it has the same issue
Please let me know if you need more information. Thank you
@rashidakanchwala if see below table
- If tags is present in url it will apply on UI
- If tags is not part of url and you have new browser(no local-storage) nothing will apply
- If tags is not part of url and and if your browser's local storage contains any tag (in your case 'companies') it will applies from your browser's local storage.
this is expected behaviour.
okay I understand that if there is any data in local storage, we try to get that and there is no way to check if the user removed all tags. For nodeTypes we do have some default but for tags we do not have such thing and that makes this test case confusing. Basically if there is local storage, the user can only disable a tag via UI and not via URL. May be we need to think about this approach. It is a bit confusing if I want to change the URL and it keeps appending tags.
http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false
Historically, parameters in the flowchart are hidden by default based on user research indicating that most users do not need to see them. This approach has not drawn complaints, suggesting it is generally acceptable. However if we see a lot of users complaining, maybe we can consider revising our approach. I think, when node types are unspecified, the URL should automatically revert to the default view, which includes both task and data types.
Hi @jitu5 , Appreciate your patience 💯 I am testing the below scenario -
- I have a tag filter with companies selected
- I go to incognito and paste the url - it works fine
- I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
- I tested the scenario on normal browser and it has the same issue
Please let me know if you need more information. Thank you
[ ![stateful_test1](https://private-user-images.githubusercontent.com/87735534/320918972-3527cf61-8813-4208-af23-2b3b85543b87.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTI2Nzg4NjIsIm5iZiI6MTcxMjY3ODU2MiwicGF0aCI6Ii84NzczNTUzNC8zMjA5MTg5NzItMzUyN2NmNjEtODgxMy00MjA4LWFmMjMtMmIzYjg1NTQzYjg3LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDA5VDE2MDI0MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU1ZmZlZGE5YzAxMmJhNzQxMTY5YjIzYjVlOGI0NWRkNDEyZTQxOGY0Y2Q2MWIzZmM0OTNhZWQ5OGMwZWI0Y2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1EHhJ9fXU64gtnbEkcyVPvKMFtaNldbbPfpckv33jfY) ](https://private-user-images.githubusercontent.com/87735534/320918972-3527cf61-8813-4208-af23-2b3b85543b87.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTI2Nzg4NjIsIm5iZiI6MTcxMjY3ODU2MiwicGF0aCI6Ii84NzczNTUzNC8zMjA5MTg5NzItMzUyN2NmNjEtODgxMy00MjA4LWFmMjMtMmIzYjg1NTQzYjg3LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDA5VDE2MDI0MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU1ZmZlZGE5YzAxMmJhNzQxMTY5YjIzYjVlOGI0NWRkNDEyZTQxOGY0Y2Q2MWIzZmM0OTNhZWQ5OGMwZWI0Y2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1EHhJ9fXU64gtnbEkcyVPvKMFtaNldbbPfpckv33jfY) [ ](https://private-user-images.githubusercontent.com/87735534/320918972-3527cf61-8813-4208-af23-2b3b85543b87.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTI2Nzg4NjIsIm5iZiI6MTcxMjY3ODU2MiwicGF0aCI6Ii84NzczNTUzNC8zMjA5MTg5NzItMzUyN2NmNjEtODgxMy00MjA4LWFmMjMtMmIzYjg1NTQzYjg3LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDA5VDE2MDI0MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU1ZmZlZGE5YzAxMmJhNzQxMTY5YjIzYjVlOGI0NWRkNDEyZTQxOGY0Y2Q2MWIzZmM0OTNhZWQ5OGMwZWI0Y2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1EHhJ9fXU64gtnbEkcyVPvKMFtaNldbbPfpckv33jfY)
it does seem a bit strange that users wont be able to tweak the URL themselves. What happens if the user clicks on the Companies tag to remove it from the UI, will the URL update?
@stephkaiser User can modify URL themselves. but wherever you select the tag or nodeType it updates the url as well as it update your browser's local storage. So whenever you hit URL without tag it will check for for your browser's local storage if any previously applied tag is present if yes, it applies on UI as well as it update URL
@jitu5 - I faced an issue related to tags that I got in the beginning but couldn't do it again whilst testing that case Ravi mentioned above.
companies
tag. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies
evaluate
tag. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies,evaluate
tag group
deselect. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=shuttles,train
I think it should revert it back to no tags.
Team, I propose we meet tomorrow after the standup to discuss our approach to tags. Given our current way of saving, reading to local storage and reading from the URL, some of design decisions have been made that were thought most optimal while Steph was away. Let's see what we can release soon and identify areas for future improvement.
I also had a question regarding the types - what are the task and data types? From the UI i see we have Nodes, Datasets, or Parameters so i'm a bit confused.
task = node, but from user perspective, I agree we should keep the language consistent. so we should say type = data,node,parameters.
I think a meeting to discuss this work would be great, i wasn't involved in this so would be great to get a walkthrough and discuss tags and the default view. Also agree we should always keep language consistent, I would even suggest type = datasets,nodes,parameters.
Please sign me up to the meeting!
@jitu5 - I faced an issue related to tags that I got in the beginning but couldn't do it again whilst testing that case Ravi mentioned above.
- Open the browser
- Select
companies
tag. Updates URL tohttp://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies
- Select
evaluate
tag. Updates URL tohttp://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies,evaluate
- Now click on the
tag group
deselect. Updates URL tohttp://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=shuttles,train
I think it should revert it back to no tags.
@rashidakanchwala I was able to reproduce this issue with group tag filter, yesterday I fix for this and now you can test it again.
I think a meeting to discuss this work would be great, i wasn't involved in this so would be great to get a walkthrough and discuss tags and the default view. Also agree we should always keep language consistent, I would even suggest type = datasets,nodes,parameters.
@stephkaiser, Now types in URL matches with UI labels type=datasets,nodes,parameters
thanks.
I'm not qualified to comment on the code and also unfortunately don't have a lot of time to do manual QA of this in the face of #1764. Please don't wait on my review 🙏🏼 Thanks @jitu5 !
Description
Resolves #1736
In this PR the stateful URLs to include additional functionalities such as
type=task,data
)tags=companies,evaluate
)expandAllPipelines=true
)Below are four scenarios where the possible values for new query parameters could be:
https://github.com/kedro-org/kedro-viz/assets/38945204/7884e73e-27b6-460d-a7f6-a5bbd59eb1e8
Development notes
pipeline_id
,expandAllPipelines
andtypes
will always be part of url.URLSearchParams
used instead ofmatchPath
.QA notes
Checklist
RELEASE.md
file