Refactor / node list row #2143

Huongg · 2024-10-16T09:06:19Z

Description

Development notes

Initially, the node-list-row component was used in three different contexts:

node-list-tree: (labelled as 3 in the chart below) representing the list of nodes
node-list-group: (labelled as 4A) is the parent level within the filter panel (e.g., Element types, Tags)
node-row-list: (labelled as 4B) is the child level within the filter panel (e.g., nodes, datasets, parameters)

The new approach involves splitting node-list-tree into two distinct components:

row: dedicated to the hierarchical structure of node elements
filter-row: dedicated to the filter panel
Additionally, a ToggleControl component is shared between these two components. This simple component renders an icon with the toggle functionality to turn on/off the functionality

QA notes

Please check the changes locally and test throughout to see if any changes that would affect the functionality. I'm currently updating some of the cypress tests which currently failed due to changes in classnames.

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

Signed-off-by: Huong Nguyen <huongg1409@gmail>

rashidakanchwala · 2024-10-18T14:44:11Z

src/components/filter-row/filter-row.js

@@ -0,0 +1,65 @@
+import React from 'react';
+import classnames from 'classnames';
+import VisibleIcon from '../icons/visible';


the filter row doesn't have the eye icon, so why are we calling it here?

hey @rashidakanchwala I've updated it to IndicatorIcon instead as its for the FilterRow specifically now

rashidakanchwala · 2024-10-18T15:04:42Z

src/components/filter-row/filter-row.js

+import './filter-row.scss';
+
+export const FilterRow = ({
+  allUnchecked,


there's some problem here, when I uncheck everything, the visibility icons disappear.

it should be fixed now, the PR is ready for reviewing the UI parts now 😄

Signed-off-by: Huong Nguyen <huongg1409@gmail>

Huongg · 2024-10-28T11:00:26Z

hey @jitu5 good spot, thank you. I missed out the :hover on the row css. I applied them now and it should fixed your concern. Let me know if you spot anything :)

rashidakanchwala · 2024-10-28T16:18:53Z

src/components/node-list/node-list-group.js

@@ -35,31 +35,29 @@ export const NodeListGroup = ({
      )}
    >
      <h3 className="pipeline-nodelist__heading">
-        <NodeListRow
+        <FilterRow


I am not sure if we need all ul, li code now ? it works without it because of FilterRow

hey @rashidakanchwala thanks for spotting this, as discussed I've removed all the un-used li elements, and add an extra props to FilterRow to determine whether it should be a div or li element. The HTML structure will now look something like in the screenshot here

rashidakanchwala · 2024-10-28T16:19:35Z

src/components/node-list/node-list-row-list.js

-          <NodeListRow
-            container="li"
-            key={item.id}
+          <FilterRow


same here, why do we need the ul/li code now, since FilterRow is a div. when I inspect the code, it looks a bit confusing?

same as above

Signed-off-by: Huong Nguyen <huongg1409@gmail>

…ss as nolonger used Signed-off-by: Huong Nguyen <huongg1409@gmail>

Signed-off-by: Huong Nguyen <huongg1409@gmail>

rashidakanchwala · 2024-10-29T09:12:46Z

src/components/filter-row/filter-row.scss

+  }
+
+  // Bright row text when all unchecked
+  .pipeline-nodelist__group--all-unchecked & {


why isn't this filter-row-all-unchecked ?

Hey, this one’s a bit unusual, but it's related to this issue. When all the boxes are unchecked, the opacity should stay the same, as it currently does on the demo site. To achieve this, I need to apply styling at the parent level. It might actually make sense for this styling to be handled in the parent component instead. Let me move it over there.

rashidakanchwala

Mostly looks good to me. It's a bit difficult to review because it's only partial refactor, so it will start making more sense when it's all done. I shall approve it once tests pass. thanks @Huongg

Signed-off-by: Huong Nguyen <huongg1409@gmail>

Huongg · 2024-10-29T13:16:09Z

thanks @rashidakanchwala, i've updated all the tests and also changed the base branch to a feature-branch instead. Let me know if you're happy for me to merge?

Also @jitu5 happy for me to merge to feature-branch?

jitu5 · 2024-10-29T13:39:33Z

thanks @rashidakanchwala, i've updated all the tests and also changed the base branch to a feature-branch instead. Let me know if you're happy for me to merge?

Also @jitu5 happy for me to merge to feature-branch?

@Huongg Related to hover, I tested on your latest branch. Its very minor things, the initial part of the row is not getting highlighted. May be not the blocker to merge.

Signed-off-by: Huong Nguyen <huongg1409@gmail>

Huongg · 2024-10-29T14:09:27Z

@Huongg Related to hover, I tested on your latest branch. Its very minor things, the initial part of the row is not getting highlighted. May be not the blocker to merge.

Ah I just realised It caused by the MUI, therefore I'm applying the styling to override. It should be okay when hovering over now. Let me know what you think? Thank you for raising it @jitu5

Signed-off-by: Huong Nguyen <huongg1409@gmail>

jitu5 · 2024-10-29T14:14:48Z

@Huongg Related to hover, I tested on your latest branch. Its very minor things, the initial part of the row is not getting highlighted. May be not the blocker to merge.

Ah I just realised It caused by the MUI, therefore I'm applying the styling to override. It should be okay when hovering over now. Let me know what you think? Thank you for raising it @jitu5

All good now. Thanks @Huongg

Signed-off-by: Huong Nguyen <huongg1409@gmail>

* Refactor / node list row (#2143) * update classnames to match the component name Signed-off-by: Huong Nguyen <huongg1409@gmail> * update names in tests Signed-off-by: Huong Nguyen <huongg1409@gmail> * update the rest of the classnames Signed-off-by: Huong Nguyen <huongg1409@gmail> * abstract node-list-row-toggle component Signed-off-by: Huong Nguyen <huongg1409@gmail> * tidy up code for toggle component Signed-off-by: Huong Nguyen <huongg1409@gmail> * update classnames in tests Signed-off-by: Huong Nguyen <huongg1409@gmail> * simplify the css Signed-off-by: Huong Nguyen <huongg1409@gmail> * add tests for node-list-row-toggle Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove handleToggle on VisibilityIcon Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove redux from node-list-row Signed-off-by: Huong Nguyen <huongg1409@gmail> * split node-list-row into row and filter-row Signed-off-by: Huong Nguyen <huongg1409@gmail> * rename toggle icon component Signed-off-by: Huong Nguyen <huongg1409@gmail> * move row and filter-row to components level Signed-off-by: Huong Nguyen <huongg1409@gmail> * move css to row and filterRow Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove node-list-row Signed-off-by: Huong Nguyen <huongg1409@gmail> * separate the row-text component Signed-off-by: Huong Nguyen <huongg1409@gmail> * include parent classname Signed-off-by: Huong Nguyen <huongg1409@gmail> * update name for toggle-icon, to visibility-control Signed-off-by: Huong Nguyen <huongg1409@gmail> * fix css and move nodeListRowHeight to config Signed-off-by: Huong Nguyen <huongg1409@gmail> * adding test for new component Signed-off-by: Huong Nguyen <huongg1409@gmail> * update classname for tests Signed-off-by: Huong Nguyen <huongg1409@gmail> * move row inside node-list Signed-off-by: Huong Nguyen <huongg1409@gmail> * connect redux store to component Signed-off-by: Huong Nguyen <huongg1409@gmail> * fix styling Signed-off-by: Huong Nguyen <huongg1409@gmail> * update name to ToggleControl Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove disable props as no longer needed Signed-off-by: Huong Nguyen <huongg1409@gmail> * replace js code with css to simplify the code Signed-off-by: Huong Nguyen <huongg1409@gmail> * update classnames in cypress test Signed-off-by: Huong Nguyen <huongg1409@gmail> * Styling for hovering and focus mode Signed-off-by: Huong Nguyen <huongg1409@gmail> * fixing small styling Signed-off-by: Huong Nguyen <huongg1409@gmail> * fix the disable styling for row Signed-off-by: Huong Nguyen <huongg1409@gmail> * fix the disable styling on focus mode Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove one of the old test Signed-off-by: Huong Nguyen <huongg1409@gmail> * update name for icons for FilterRow Signed-off-by: Huong Nguyen <huongg1409@gmail> * fixing the icon highlighting issue Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove un-used li element Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove styling for pipeline-nodelist__placeholder-upper and lower class as nolonger used Signed-off-by: Huong Nguyen <huongg1409@gmail> * update test in node-list Signed-off-by: Huong Nguyen <huongg1409@gmail> * update cypress tests Signed-off-by: Huong Nguyen <huongg1409@gmail> * moving .pipeline-nodelist__group--all-unchecked to the parent Signed-off-by: Huong Nguyen <huongg1409@gmail> * prevent page reload on form submission Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove wrong classname in the test Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove unique ID Signed-off-by: Huong Nguyen <huongg1409@gmail> * apply hovering styling on the parent instead of row Signed-off-by: Huong Nguyen <huongg1409@gmail> * styling for selected element Signed-off-by: Huong Nguyen <huongg1409@gmail> * fixing hover styling on the icon from MUI Signed-off-by: Huong Nguyen <huongg1409@gmail> --------- Signed-off-by: Huong Nguyen <huongg1409@gmail> Co-authored-by: Huong Nguyen <huongg1409@gmail> * Refactor/node list groups (#2166) * Create new structure and its own folder for filters or groups Signed-off-by: Huong Nguyen <huongg1409@gmail> * better names for component structure Signed-off-by: Huong Nguyen <huongg1409@gmail> * FiltersSectionHeading Signed-off-by: Huong Nguyen <huongg1409@gmail> * filters-section Signed-off-by: Huong Nguyen <huongg1409@gmail> * filters component Signed-off-by: Huong Nguyen <huongg1409@gmail> * filtersSectionHeading component Signed-off-by: Huong Nguyen <huongg1409@gmail> * tidy up code Signed-off-by: Huong Nguyen <huongg1409@gmail> * including new tests for new components Signed-off-by: Huong Nguyen <huongg1409@gmail> * update and remove existing tests Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove un-used variables Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove components folder Signed-off-by: Huong Nguyen <huongg1409@gmail> * update tests path Signed-off-by: Huong Nguyen <huongg1409@gmail> --------- Signed-off-by: Huong Nguyen <huongg1409@gmail> Co-authored-by: Huong Nguyen <huongg1409@gmail> * Refactor/node list index (#2178) * foundation for FiltersContext Signed-off-by: Huong Nguyen <huongg1409@gmail> * remove unused props Signed-off-by: Huong Nguyen <huongg1409@gmail> * node-list-context Signed-off-by: Huong Nguyen <huongg1409@gmail> * restructure node-list-item as a helper function Signed-off-by: Huong Nguyen <huongg1409@gmail> * rename selectors Signed-off-by: Huong Nguyen <huongg1409@gmail> * rename functions in FiltersContext Signed-off-by: Huong Nguyen <huongg1409@gmail> * move redux selector to node-list-context Signed-off-by: Huong Nguyen <huongg1409@gmail> * fixing the hovered node issue Signed-off-by: Huong Nguyen <huongg1409@gmail> * move getFilteredItems to selector Signed-off-by: Huong Nguyen <huongg1409@gmail> * fix the modularpipeline highlight issue Signed-off-by: Huong Nguyen <huongg1409@gmail> * Adding test for selector Signed-off-by: Huong Nguyen <huongg1409@gmail> * update tests Signed-off-by: Huong Nguyen <huongg1409@gmail> * update names to be nodes-panel Signed-off-by: Huong Nguyen <huongg1409@gmail> * Fixing the filters problem Signed-off-by: Huong Nguyen <huongg1409@gmail> * update test Signed-off-by: Huong Nguyen <huongg1409@gmail> * fixing the highlight issue through getNodesActive Signed-off-by: Huong Nguyen <huongg1409@gmail> * move node-list-tree to its own component Signed-off-by: Huong Nguyen <huongg1409@gmail> * update row to node-list-row Signed-off-by: Huong Nguyen <huongg1409@gmail> * move style to be inside node-list-tree Signed-off-by: Huong Nguyen <huongg1409@gmail> * fix the filters URL update Signed-off-by: Huong Nguyen <huongg1409@gmail> * update name for nodes panel context Signed-off-by: Huong Nguyen <huongg1409@gmail> --------- Signed-off-by: Huong Nguyen <huongg1409@gmail> Co-authored-by: Huong Nguyen <huongg1409@gmail> * Refactor on NodeListTree (#2177) This is a smaller refactor. Previously, the logic for determining which nodes were disabled due to modular pipelines was duplicated in both the NodeListTree component and the getNodeDisabled selector. To improve maintainability and reduce redundancy, the getnodesDisabledViaModularPipeline logic was extracted and made into it's own selector. Now, this logic is shared and reused by both the NodeListTree component and the getNodeDisabled selector. * fixed issue with nested focus modular pipeline Signed-off-by: rashidakanchwala <[email protected]> * Update RELEASE.md Signed-off-by: rashidakanchwala <[email protected]> * Update .telemetry Signed-off-by: rashidakanchwala <[email protected]> * Update pyproject.toml Signed-off-by: rashidakanchwala <[email protected]> * Update apps.py Signed-off-by: rashidakanchwala <[email protected]> * Update telemetry.html Signed-off-by: rashidakanchwala <[email protected]> * fix issue around parent pipelines disabled child pipelines Signed-off-by: rashidakanchwala <[email protected]> * include in the release note Signed-off-by: Huong Nguyen <huongg1409@gmail> * fixing the padding bottom gap for filters Signed-off-by: Huong Nguyen <huongg1409@gmail> --------- Signed-off-by: Huong Nguyen <huongg1409@gmail> Signed-off-by: rashidakanchwala <[email protected]> Signed-off-by: rashidakanchwala <[email protected]> Co-authored-by: Huong Nguyen <huongg1409@gmail> Co-authored-by: rashidakanchwala <[email protected]> Co-authored-by: rashidakanchwala <[email protected]>

Huong Nguyen added 21 commits October 15, 2024 16:16

update classnames to match the component name

63431e4

Signed-off-by: Huong Nguyen <huongg1409@gmail>

update names in tests

e1ab740

Signed-off-by: Huong Nguyen <huongg1409@gmail>

update the rest of the classnames

976ff98

Signed-off-by: Huong Nguyen <huongg1409@gmail>

abstract node-list-row-toggle component

e5350ad

Signed-off-by: Huong Nguyen <huongg1409@gmail>

tidy up code for toggle component

988ca60

Signed-off-by: Huong Nguyen <huongg1409@gmail>

update classnames in tests

5d37a5b

Signed-off-by: Huong Nguyen <huongg1409@gmail>

simplify the css

abf00c8

Signed-off-by: Huong Nguyen <huongg1409@gmail>

add tests for node-list-row-toggle

3c2d623

Signed-off-by: Huong Nguyen <huongg1409@gmail>

remove handleToggle on VisibilityIcon

479701e

Signed-off-by: Huong Nguyen <huongg1409@gmail>

remove redux from node-list-row

39c8db8

Signed-off-by: Huong Nguyen <huongg1409@gmail>

split node-list-row into row and filter-row

ca8b72c

Signed-off-by: Huong Nguyen <huongg1409@gmail>

rename toggle icon component

65382f8

Signed-off-by: Huong Nguyen <huongg1409@gmail>

move row and filter-row to components level

9162c10

Signed-off-by: Huong Nguyen <huongg1409@gmail>

move css to row and filterRow

b174662

Signed-off-by: Huong Nguyen <huongg1409@gmail>

remove node-list-row

d87c2b2

Signed-off-by: Huong Nguyen <huongg1409@gmail>

separate the row-text component

a7ef9b6

Signed-off-by: Huong Nguyen <huongg1409@gmail>

include parent classname

433d222

Signed-off-by: Huong Nguyen <huongg1409@gmail>

update name for toggle-icon, to visibility-control

c51deaa

Signed-off-by: Huong Nguyen <huongg1409@gmail>

fix css and move nodeListRowHeight to config

3774aec

Signed-off-by: Huong Nguyen <huongg1409@gmail>

adding test for new component

9c341ab

Signed-off-by: Huong Nguyen <huongg1409@gmail>

update classname for tests

46830b8

Signed-off-by: Huong Nguyen <huongg1409@gmail>

rashidakanchwala reviewed Oct 18, 2024

View reviewed changes

Huong Nguyen added 7 commits October 22, 2024 12:13

move row inside node-list

c9bd1bb

Signed-off-by: Huong Nguyen <huongg1409@gmail>

connect redux store to component

80428af

Signed-off-by: Huong Nguyen <huongg1409@gmail>

fix styling

55f6e3f

Signed-off-by: Huong Nguyen <huongg1409@gmail>

update name to ToggleControl

d6e493e

Signed-off-by: Huong Nguyen <huongg1409@gmail>

remove disable props as no longer needed

b9fb79b

Signed-off-by: Huong Nguyen <huongg1409@gmail>

replace js code with css to simplify the code

01f2498

Signed-off-by: Huong Nguyen <huongg1409@gmail>

update classnames in cypress test

864b6cd

Signed-off-by: Huong Nguyen <huongg1409@gmail>

fixing the icon highlighting issue

cdd10f1

Signed-off-by: Huong Nguyen <huongg1409@gmail>

rashidakanchwala assigned Huongg Oct 28, 2024

rashidakanchwala reviewed Oct 28, 2024

View reviewed changes

Huong Nguyen added 3 commits October 28, 2024 16:46

remove un-used li element

d3c16ae

Signed-off-by: Huong Nguyen <huongg1409@gmail>

remove styling for pipeline-nodelist__placeholder-upper and lower cla…

3da4864

…ss as nolonger used Signed-off-by: Huong Nguyen <huongg1409@gmail>

update test in node-list

db9a947

Signed-off-by: Huong Nguyen <huongg1409@gmail>

rashidakanchwala reviewed Oct 29, 2024

View reviewed changes

Huong Nguyen added 6 commits October 29, 2024 11:34

update cypress tests

e1c2dff

Signed-off-by: Huong Nguyen <huongg1409@gmail>

moving .pipeline-nodelist__group--all-unchecked to the parent

8ef75b8

Signed-off-by: Huong Nguyen <huongg1409@gmail>

prevent page reload on form submission

0abbf59

Signed-off-by: Huong Nguyen <huongg1409@gmail>

remove wrong classname in the test

db25f03

Signed-off-by: Huong Nguyen <huongg1409@gmail>

remove unique ID

cb342b5

Signed-off-by: Huong Nguyen <huongg1409@gmail>

Merge branch 'main' into refactor/node-list-row

7891403

Huongg changed the base branch from main to feature-branch/refactor-node-list October 29, 2024 13:14

apply hovering styling on the parent instead of row

faac318

Signed-off-by: Huong Nguyen <huongg1409@gmail>

styling for selected element

932b89a

Signed-off-by: Huong Nguyen <huongg1409@gmail>

jitu5 approved these changes Oct 29, 2024

View reviewed changes

rashidakanchwala approved these changes Oct 29, 2024

View reviewed changes

fixing hover styling on the icon from MUI

aaf9172

Signed-off-by: Huong Nguyen <huongg1409@gmail>

Huongg merged commit 2c36a43 into feature-branch/refactor-node-list Oct 29, 2024
4 checks passed

Huongg deleted the refactor/node-list-row branch October 29, 2024 14:44

Huongg mentioned this pull request Nov 13, 2024

Refactor/node list index #2178

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor / node list row #2143

Refactor / node list row #2143

Huongg commented Oct 16, 2024 •

edited

Loading

rashidakanchwala Oct 18, 2024

Huongg Oct 24, 2024

rashidakanchwala Oct 18, 2024

Huongg Oct 24, 2024 •

edited

Loading

Huongg commented Oct 28, 2024

rashidakanchwala Oct 28, 2024

Huongg Oct 28, 2024

rashidakanchwala Oct 28, 2024

Huongg Oct 28, 2024

rashidakanchwala Oct 29, 2024

Huongg Oct 29, 2024

rashidakanchwala left a comment •

edited

Loading

Huongg commented Oct 29, 2024

jitu5 commented Oct 29, 2024

Huongg commented Oct 29, 2024

jitu5 commented Oct 29, 2024

Refactor / node list row #2143

Refactor / node list row #2143

Conversation

Huongg commented Oct 16, 2024 • edited Loading

Description

Development notes

QA notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huongg Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Huongg commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashidakanchwala left a comment • edited Loading

Choose a reason for hiding this comment

Huongg commented Oct 29, 2024

jitu5 commented Oct 29, 2024

Huongg commented Oct 29, 2024

jitu5 commented Oct 29, 2024

Huongg commented Oct 16, 2024 •

edited

Loading

Huongg Oct 24, 2024 •

edited

Loading

rashidakanchwala left a comment •

edited

Loading