[DRAFT] Refactor/node list index #2178
Conversation
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
Signed-off-by: Huong Nguyen <huongg1409@gmail>
There was a problem hiding this comment.
There are selectors in this file that can be moved to selector folder
There was a problem hiding this comment.
I think some functions live in utils and others in selectors so maybe we can move it accordingly?
| handleKeyDown, | ||
| } = useContext(NodeListContext); | ||
|
|
||
| const modularPipelinesSearchResult = searchValue |
There was a problem hiding this comment.
this can be moved to node-list-tree
There was a problem hiding this comment.
the redux connection in this file can also be moved to the NodeListContextProvider , i suppose?
Huongg
changed the base branch from
main
to
feature-branch/refactor-node-list
|
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 |
| isResetFilterActive={isResetFilterActive} | ||
| /> | ||
| <AppContextProvider> | ||
| <NodeList faded={faded} /> |
There was a problem hiding this comment.
Using context looks great. One question I have is does NodeList gets rendered everytime there is a change in AppContext whether or not NodeList depends on the change ? I see AppContext has NodeListContextProvider. A bit confused on why can't we have -
<NodeListContextProvider>
<NodeList faded={faded} />Thank you
There was a problem hiding this comment.
Hey, yes, the NodeList component does re-render every time there's a change in AppContext. However, even without context when we just pass everything down as a props through index.js, it will re-render whenever its props change. Good point, though! We could use React.memo to optimise it, but that would be independent of useContext
There was a problem hiding this comment.
without wrapping it around AppContext it will look like this. I thought it looks nicer with just one AppContext. WDYT?
<NodeListContextProvider>
<FiltersContextProvider>
<NodeList faded={faded} />
....
| focusMode, | ||
| disabledModularPipeline, | ||
| handleKeyDown, | ||
| } = useContext(NodeListContext); |
| onGroupToggleChanged={handleGroupToggleChanged} | ||
| onItemChange={handleFiltersRowClicked} | ||
| onResetFilter={handleResetFilter} | ||
| onToggleGroupCollapsed={handleToggleGroupCollapsed} |
There was a problem hiding this comment.
[Nit] I know the PR is still in draft. If we want to have the components - SearchList, NodeListTree, Filters in this file, this file needs to be shifted to a UI section or needs to be renamed. As node-list.js suggests only the list of nodes we see in the pipeline-sidebar. Wdyt @Huongg
There was a problem hiding this comment.
Agree. Maybe we could rename as follows
Parent component could be called Node Explorer, and within that we have SearchList, NodeListTree, and Filters -- all of these could be top level components, i think both SearchList and Fitlers are already and NodeListTree can be too.
|
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 💯 |
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.
QA notes
This isn't finalised yet, as some tests are still failing, new ones need to be added, and a few minor bugs have arisen from the changes. This is a work in progress, so please focus only on reviewing the overall code structure.
Checklist
RELEASE.mdfile