search

kedro-org / kedro-viz

Visualise your Kedro data and machine-learning pipelines and track your experiments.
https://demo.kedro.org
Apache License 2.0
680 stars 113 forks source link

QA: Feature branch for refactor node list #2193

Closed Huongg closed 2 days ago

Huongg commented 1 week ago

Description

2192

This PR focuses on ensuring thorough QA throughout the changes for the significant refactor of the node-list, before we merge it to main branch

The final structure looks as below:

Screenshot 2024-11-15 at 11 06 08

Miro board: https://miro.com/app/board/uXjVLTeWe88=/

Development notes

QA notes

Checklist

jitu5 commented 3 days ago

@Huongg I tested locally, I observed few differences compared to demo site

  1. On demo project, when you clicks on focus icon of "train_evaluation", "Evaluate Model" and "Train Model" is disabled but on demo site its not.
Screenshot 2024-11-18 at 5 51 26 p m Screenshot 2024-11-18 at 5 50 19 p m
  1. When you click on any row on node list and then when you de-select or click any where else it removed selection but on demo site it remains highlighted.

high-w high-nw-1

rashidakanchwala commented 3 days ago

Hi @jitu5,

I’m looking into this. Here’s what I’ve found so far:

  1. On both demo.kedro.org and localhost, the grandchildren nodes are not visible on the flowchart. Additionally, on the demo site, the nodes on the nodelist are still not disabled, which is incorrect because they cannot be interacted with. However, overall, they shouldn't be disabled, so we’ll address this issue.

  2. It seems the highlight node bug is now resolved with this refactor.

jitu5 commented 3 days ago

Hi @jitu5,

I’m looking into this. Here’s what I’ve found so far:

  1. On both demo.kedro.org and localhost, the grandchildren nodes are not visible on the flowchart. Additionally, on the demo site, the nodes on the nodelist are still not disabled, which is incorrect because they cannot be interacted with. However, overall, they shouldn't be disabled, so we’ll address this issue.
  2. It seems the highlight node bug is now resolved with this refactor.

Hi @rashidakanchwala For point 1 now its fixed for the grandchildren nodes but I notice one more issue, when I click a focus icon for "train_evaluation.linear_regression" now I cant un-do focus when I click on the same focus icon again, even I cant collapse or expand "train_evaluation.linear_regression"

child-par

For point 2, still its not retaining highlight.

Huongg commented 3 days ago

hey @jitu5 retaining highlighted when de-selecting on a node is a bug on the demo site, which is addressed with this refactoring. So when you click on a node, it should highlight, when not clicking on a node, it shouldn't highlight

jitu5 commented 3 days ago

when I click a focus icon for "train_evaluation.linear_regression" now I cant un-do focus when I click on the same focus icon again, even I cant collapse or expand "train_evaluation.linear_regression"

@rashidakanchwala Now its working as expected. Thanks

stephkaiser commented 3 days ago

great work team! I've noticed a few things:

  1. the padding space at the bottom of filters under the last tag is shorter than previous (see current vs demo site):

    Screenshot 2024-11-19 at 14 28 29 Screenshot 2024-11-19 at 14 28 18

  2. Not sure if its just me or gitpod, but im noticing some delay when I move/hover my mouse across the node list:

https://github.com/user-attachments/assets/12ad1c92-1c93-41e3-b97e-32b7602ef9e9

  1. I've noticed the hide + focus buttons on child pipelines are not positioned the same like the parent hide + focus buttons (the hide + focus buttons should always be in the same position, its currently too close to the utility bar on the right). We should shorten the label if there isn't enough space for the buttons. In the screenshot, it also looks like the spacing between the label and hide button is smaller than the space between the hide + focus buttons.

https://github.com/user-attachments/assets/385f78d1-1989-46a7-8fa2-15037ba82cb3

Screenshot 2024-11-19 at 14 26 26
  1. I've spotted something too - it's currently quite difficult to see if an item is hidden or not - unless you hover on that item and see that the hide icon has changed, the label colour also changes to a lighter grey but its very subtle. I suggest when an item is hidden, we always show the hide icon (not only on hover) until the user clicks the icon again to un-hide the item then the icon goes back to being shown only on hover (kind of the same behaviour as the focus icon except the hide icon stays grey and doesn't become blue, and it doesn't disable the other list items when activated)

https://github.com/user-attachments/assets/988d9314-70d4-45c2-81ea-94842a562fe2

  1. Another spot - could we add tooltips for 'Hide' and 'Focus' buttons? currently when hovering over these two buttons they show the name of the item, rather than showing the tooltip for the buttons.
Screenshot 2024-11-19 at 14 32 44 Screenshot 2024-11-19 at 14 32 43

Thank you!!

Huongg commented 3 days ago

Hi @stephkaiser, thanks for reviewing this! I discussed with @rashidakanchwala, and we agreed that only the first issue is introduced by the refactored code, which I’ve now addressed.

Screenshot 2024-11-19 at 16 24 57

Regarding point 2, I suspect it might be related to the Gitpod setup, as I’m seeing the hover state working fine on my end.

The remaining issues are not introduced from the refactoring work, for example, point 3 is an existing bug (it’s also visible on the demo site when "pretty name" is turned off), and points 4 and 5 are new enhancements. I’ve combined these into a single ticket: https://github.com/kedro-org/kedro-viz/issues/2197.

If you're happy with this suggestion, can we merge this so it won't delay the release?

stephkaiser commented 2 days ago

@Huongg sounds good to me! thanks Huong. I just checked the first issue and it looks good now, thank you!