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

Refactor/node list index #2178

Closed Huongg closed 1 week ago

Huongg commented 2 weeks ago

Description

Original issue: The index file had grown significantly, with numerous if/else statements due to shared functionality between node-list and filters.

I've now split the logic into two separate contexts: node-list-context, specifically for the node list, and filters-context, dedicated to filters. Each context now contains its own selectors, actions, and state, tailored for its specific purpose, along with related functionality like itemChanged and itemClicked. This has removed the need for shared logic, making the codebase more modular and maintainable.

Screenshot 2024-11-13 at 09 51 26

Checklist

rashidakanchwala commented 2 weeks ago

I had a first pass, and I love it to so much! thanks @Huongg , i am also new to useContext, so reading about it. It makes it so much cleaner <3

ravi-kumar-pilla commented 2 weeks ago

Hi @Huongg , Great work. I had a brief look at the overall structure. As @rashidakanchwala mentioned, it looks more readable having context passed to children. I left few comments. My suggestion would be to pass only the required context to the children components to avoid re-rendering. Thank you đź’Ż

jitu5 commented 1 week ago

@Huongg When testing locally, I noticed a bug related to the filter. When you select any filter (try using tags), it gets updated in the URL. However, if you deselect the same filter, it does not get removed from the URL.

Please notice when I click on "companies" tag. filter-issue

Huongg commented 1 week ago

thanks for spotting it, and walking me through the issue. @jitu5. I missed the function handleUrlParamsUpdateOnFilter when refactoring the code. I believe it should be working as usual now. Do let me know if its still not right from your side. I've also reported a separate issue we found on the call together https://github.com/kedro-org/kedro-viz/issues/2186

jitu5 commented 1 week ago

@Huongg, I had a brief look at the PR, and it looks really good, especially the use of React context, even though I am still a bit new to it. I retested the filter issue, and it’s now fixed—thanks for that!

However, I am a bit confused about the component naming and the parent-child connections, as some new components have been introduced and some existing ones have been renamed from your previous PR. It would be really great if you could add an updated flowchart or block diagram to this PR.

Overall this is excellent work. 🎉

ravi-kumar-pilla commented 1 week ago

Hi @Huongg, I see all the big refactor and tons of work you have put in 🥇 . It would be helpful to review and revisit, if there is a flowchart explaining -

  1. Grouped components
  2. Parent components
  3. Children components
  4. Flow of data from parent <-> child - This can be a simple example of what happens if a pipeline dropdown is changed or someone searches in node search or someone filters
  5. Flow of data if any from redux <-> components - This can be a simple example of what happens if a pipeline dropdown is changed or someone searches in node search or someone filters
  6. Context providers
  7. Any particular part of code you feel is the biggest change - For example use of ContextProviders
  8. Component re-renders when NodesPanel state changes

Thank you

Huongg commented 1 week ago

hey @ravi-kumar-pilla and @jitu5 I've revised the diagram to reflect the updated component structure.

Screenshot 2024-11-13 at 09 51 26

with details miro board: https://miro.com/app/board/uXjVLTeWe88=/

Here's an overview of the new structure:

  1. nodes-panel: This serves as the parent component, importing the other three components—search-list, node-list-tree, and filters. It includes two context providers:

    • node-list-context: Focused on the node list, handling selectors, actions, and state specific to nodes.
    • filters-context: Dedicated to filters, with selectors, actions, and state for managing filter functionality. Both contexts support related functions like itemChanged and itemClicked.
  2. node-list-tree: This renders the tree list of nodes and modular pipelines. This work is currently part of a separate ticket (#2177). Per @rashidakanchwala, this shouldn’t involve major changes to the existing components.

  3. filters: Responsible for rendering the filters list.

  4. Data Flow: The nodes-panel parent component manages everything, with logic split between the two contexts mentioned above. A shared state allows data to flow between the search input, node-list-tree, and filters.

  5. The most significant update in this PR is the context setup, though two earlier PRs—#2143 and #2166—introduced other substantial changes as part of a phased refactor.

  6. Components will re-render as the state changes. Let me know if there's a specific part you’d like more detail on.

Happy to get on a call if you want to further discuss about this?