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>

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>

Huongg mentioned this pull request Oct 25, 2024

Refactor NodeList Component #2037

Open

5 tasks

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