[KED-2770] - Expose related dataset nodes for focus mode flowchart #530

studioswong · 2021-08-05T00:46:17Z

Description

related ticket: https://jira.quantumblack.com/browse/KED-2770

This is the work to expose the related input and output nodes and display them with a dotted outline and edges. ( see below). Those nodes are displayed for indicative purposes only, where all typical click interations for nodes on the flowchart and sidebar will remain disabled for those input and output nodes.

Development notes

The main changes to enable this feature includes:

updating the getNodeDisabledModularPipeline selector so that it will check for input and output nodes related to the selected modular pipeline and enable that node
constructing selectors to return the list of input / output nodes and edges for reference
refactoring the focusMode state from a local state to a global state value stored under the visible prop
refactoring node-list to take in the new global state for focusMode and the related selectors for the input / output nodes and edges
added in new actions to allow the update of the focusMode global state
adding in the new css classes for the new display of those input / output nodes
adding in new changes for the flowchart draw function to assign the newly added css classes for those input / output nodes on the flowchart
Adding in checks for the sidebar node-list to check for these input / output nodes in focus mode and ensure the nodes remains disabled on the sidebar
added all relevant tests for the newly added selectors, actions, and checks throughout the codebase

I have also added in some minor refactor of the focus mode icon.

Please note that, due to the latest decision of removing the newparams flag, I have removed all considerations of styling for when newparams flag is off ( i.e old version of parameters).

QA notes

Please play around with the focus mode and toggle between filters to test the display of those input / output nodes.

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

Legal notice

I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

… of node type IDs listed in the state

…on on tree rows

…ated nested list item error on console

… along with count for listed results

… nodes

…of newParamsFlag

…into main

…within node-list-items

…into main

rashidakanchwala · 2021-08-09T13:23:51Z

src/selectors/nodes.test.js

@@ -370,4 +376,17 @@ describe('Selectors', () => {
      expect(Object.keys(nodesWithInputParams)).toEqual(parameterNodesID);
    });
  });
+
+  describe('getInputOutputDataNodes', () => {
+    it('includes input output ndoes related to a modular pipeline in the returned object', () => {


typo - nodes

Suggestion: You can use github suggestion to suggest changes for these small issues like this:

Suggested change

it('includes input output ndoes related to a modular pipeline in the returned object', () => {

it('includes input output nodes related to a modular pipeline in the returned object', () => {

Thanks both - great spot! I've updated the typo in my latest commit

limdauto

This PR is 🔥 Everything seems very clear to me. Amazing work @studioswong Just a few minor nitpicks.

I will do a bit more testing but the code looks good to me.

limdauto · 2021-08-09T15:02:14Z

src/components/flowchart/draw.js

-  if (changed('edges')) {
-    enterEdges
-      .append('path')
+  if (changed('edges', 'newParamsFlag', 'focusMode', 'inputOutputDataNodes')) {


qq: do you think we should remove newParamsFlag from this PR since its removal has already been merged? Or should that be a follow up PR?

I can remove this in this PR :) thanks for the reminder!

limdauto · 2021-08-09T15:08:11Z

src/components/flowchart/draw.js

      .attr('marker-end', (edge) =>
        edge.sourceNode.type === 'parameters'
-          ? `url(#pipeline-arrowhead--accent)`
+          ? focusMode !== null && inputOutputDataEdges[edge.id]


nit: I think giving focusMode !== null && inputOutputDataEdges[edge.id] a name might help with this block as it's repeated twice in this nested tenary operator. You can also reuse that variable further down as well.

sounds good, I'll extract that out.

Yeah agreed nested ternary ends up being pretty difficult to follow, so it's a probably a good idea to split this one up e.g. some variables / string templates, if that makes sense

limdauto · 2021-08-09T15:10:16Z

src/components/flowchart/draw.js

-        (edge) => edge.sourceNode.type === 'parameters'
+        (edge) =>
+          edge.sourceNode.type === 'parameters' &&
+          !inputOutputDataEdges[edge.id]


Sorry I don't understand why this condition !inputOutputDataEdges[edge.id] is needed here. Is it to fix the problem you identified the other day?

this condition is needed to prevent applying this class to parameter nodes that are also input/output nodes - if this check is not applied this will create potential conflicts with the css class that will be applied to parameter nodes that are part of input/output. ( i.e taking this check out will result in some input/output parameter nodes not adopting the pipeline-edge--parameters--input class. )

limdauto · 2021-08-09T15:17:22Z

src/components/flowchart/flowchart.test.js

+    );
+  });
+
+  it('applies pipeline-edge--data--input class to input dataset edges under focus mode', () => {


Nit: shouldn't this modifier be named as pipeline-edge--dataset--input to be consistent with pipeline-node--dataset-input (or the other way around)? Same comment about pipeline-edge--parameters--input and pipeline-node--parameter-input (although I have a feeling this naming is intentional because pipeline-edge might have more than 1 parameters, right?)

sure, I can rename this as pipeline-edge--dataset--input to align this with the naming for the input nodes css class

limdauto · 2021-08-09T15:23:46Z

src/components/flowchart/flowchart.test.js

@@ -361,6 +415,9 @@ describe('FlowChart', () => {
      visibleSidebar: expect.any(Boolean),
      visibleCode: expect.any(Boolean),
      visibleMetaSidebar: expect.any(Boolean),
+      inputOutputDataNodes: expect.any(Object),
+      inputOutputDataEdges: expect.any(Object),
+      focusMode: expect.any(Object),


This set of tests is awesome. Very clear!!! 🏆

limdauto · 2021-08-09T15:39:01Z

src/selectors/disabled.js

-      return true;
+
+      // Hide nodes that don't have at least one modular pipeline filter enabled
+      return !isNodeOfActiveModularPipeline(


This seems duplicated with the earlier if check. Should we short circuit by moving it up instead?

if (!isNodeOfActiveModularPipeline( nodeModularPipelines, nodeID, modularPipelineEnabled ) || (nodeType[nodeID] === 'task') { return false; }

Thanks for the suggestions - I had a look into this, and while the arrangement of the returned statement would stand as is (i.e we still need to return the result of the check and this cannot be combined with the check for the task node), I had extracted the check for isNodeOfActiveModularPipeline out to a variable so the check will not be duplicated and allows more readability. I had also added further comments to explain the exceptions that the if statements filters out before returning the result of this check as the final returned value. Hopefully this will be more clear and the reason behind this arranhement would be more apparent to you.

rashidakanchwala

Hi Susanna, Great work! it work's well for me!

bru5

Works really nicely! Great stuff 👏

Still a bit more for me to check out, but for now I've left some suggestions if you're interested, mostly around simplifying a few parts, if it makes sense.

bru5 · 2021-08-10T09:38:23Z

src/components/flowchart/draw.js

      .attr('marker-end', (edge) =>
        edge.sourceNode.type === 'parameters'
-          ? `url(#pipeline-arrowhead--accent)`
+          ? focusMode !== null && inputOutputDataEdges[edge.id]


Yeah agreed nested ternary ends up being pretty difficult to follow, so it's a probably a good idea to split this one up e.g. some variables / string templates, if that makes sense

bru5 · 2021-08-10T09:45:47Z

src/components/flowchart/draw.js

+          !inputOutputDataEdges[edge.id]
+      )
+      .classed(
+        'pipeline-edge--parameters--input',


Just to check since there's two -- in this classname is that to say there are two independent modifiers here? If so maybe consider splitting into two classnames I think to get more flexibility?

bru5 · 2021-08-10T09:54:33Z

src/components/node-list/index.js

@@ -69,7 +79,13 @@ const NodeListProvider = ({
    if (isGroupType(item.type) || isModularPipelineType(item.type)) {
      onGroupItemChange(item, item.checked);
      if (isModularPipelineType(item.type)) {
-        onToggleFocusMode(item);
+        if (isModularPipelineType(item.type)) {


Looks like this check might have been duplicated accidentally here?

bru5 · 2021-08-10T09:57:01Z

src/components/node-list/index.js

@@ -84,7 +100,11 @@ const NodeListProvider = ({
    if (isGroupType(item.type) || isModularPipelineType(item.type)) {


Thinking it might be a good idea at this point to split this into two separate blocks, to help avoid some nesting if possible?

I had a look at this, and given that both conditions calls onGroupItemChange(item, checked);, I think it is still better to combine that into a single if statement. The nesting is still readable, and I would rather avoid duplicate code.

bru5 · 2021-08-10T10:07:11Z

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

@@ -269,14 +274,16 @@ export const getFilteredNodeItems = createSelector(
          const disabled =
            node.disabled_tag ||
            node.disabled_type ||
-            node.disabled_modularPipeline;
+            node.disabled_modularPipeline ||
+            (focusMode !== null && !!inputOutputDataNodes[node.id]);


I guess focusMode && inputOutputDataNodes[node.id] can maybe be ok here (unless 0 is truthy for this)?

I am glad you raised this question - I am using !!inputOutputDataNodes[node.id] to force disabled to return a boolean value under this check. If we use inputOutputDataNodes[node.id] the check will return an error. ( you can also verify this by trying this out, and you will see what I meant ).

bru5 · 2021-08-10T10:13:48Z

src/components/node-list/styles/_row.scss

@@ -42,6 +42,10 @@
  height: $row-icon-size;
  fill: var(--color-text);

+  &.pipeline-row__toggle-icon--focus-checked{


Might just need a space adding near the end here

bru5 · 2021-08-10T10:15:13Z

src/selectors/disabled.js

 /**
- * Calculate whether nodes should be disabled based on their modular pipelines
+ * Calculate whether nodes should be disabled based on their modular pipelines,
+ * except related dataset nodes and


Maybe the comment got a bit cut off at the end here?

bru5 · 2021-08-10T10:17:39Z

src/selectors/disabled.js

+          nodeID,
+          modularPipelineEnabled
+        ) &&
+        (nodeType[nodeID] === 'parameters' || nodeType[nodeID] === 'data')


If the isNodeOfActiveModularPipeline might get a little expensive, then could be a good idea to move these simpler conditions to be checked first, to help reduce the cost?

I've refactored this check into a separate constant variable - please see my latest commit

bru5 · 2021-08-10T10:32:00Z

src/selectors/edges.js

@@ -48,10 +66,17 @@ export const getTransitiveEdges = createSelector(
          return;
        }
        const target = edgeTargets[edgeID];
+
+        // Further filter out connections between indicative input / output nodes under focus mode
+        const isNotInputEdge =


Wondering if it's worth considering flipping this to the positive e.g. isInputEdge to help simplify the condition (unless it's a performance thing I suppose?)

It is a performance thing as checking isNotInputEdge is more straight forward and has less cases rather than the positive situation ( this is apparent if you read through the condition - checking for isInputEdge has much more possibility rather than checking for isNotInputEdge. )

Furthermore, I don't really see the upside of the refactor work for flipping this to positive?

bru5 · 2021-08-10T10:33:13Z

src/selectors/edges.js

+    if (focusedModularPipeline !== null) {
+      visibleEdges.forEach((edge) => {
+        if (
+          !nodeModularPipelines[edge.source]?.includes(


Might be worth considering breaking out the conditions here into constants?

This check is only used once in this selector, moreover with it being very straight forward, I believe this is fine for this to stay as is.

studioswong · 2021-08-10T19:29:26Z

Thanks all for the comments! I've gone through all your comments and made the suggested changes.

studioswong

/

studioswong added 30 commits May 13, 2021 23:02

set up modular pipeline selectors for nested data structure

7a85656

set up tree list with material UI

5b3a6d8

set up tree-list selectors

e924eae

updated getGroupedNodes to include modular pipelines

adda65b

set up tree-list-search and refactor selectors to use filtered items

9c06d12

Merge branch 'main' into feature/KED-2563-collapsable-tree-component

cda4548

added in personal remarks for selectors

c0496fa

purge node modular pipelines

6eb4fff

set up filter panel and plug in existing react component for tree rows

3c52a47

further update style

c6ac80e

updated styles, icons, etc

9aece6a

update selectors to ensure node data gets arranged according to order…

f15cd16

… of node type IDs listed in the state

remove unneeded previous setup

6821a6f

clean up getNestedModularPipelines

7416c55

refactor node-list-tree with node-list-tree-item

858ec36

delete unneeded selectors in modular pipeline selectors

ab2881d

deleted further undeeded items

0567a1a

updated padding setting for shorter gap between dropdown arrow and ic…

d1c5a05

…on on tree rows

added in keys for nested items and changed Container to div to elimin…

33a7c16

…ated nested list item error on console

skipped some tests due to PR setup

c1bf6ef

further modified tests according to tree list structure

275dcab

remove modular pipeline filter

aac32f8

Merge branch 'main' into feature/tree-search-ui

efe7a20

update label to use highlighted label

6bdb42f

updated selectors and tree item row

bc8439c

Merge branch 'main' into feature/tree-search-ui

552e7d1

completed search

eaa5933

update new nested tree data selector to cater for unsorted pipelines,…

7fd6f8e

… along with count for listed results

allow expanded tree search UI

8524300

deleted unneeded title and style changes

b7f1086

studioswong added 11 commits August 2, 2021 14:22

added filtering logic to filter out transitive edges for input output…

b477beb

… nodes

fix focus mode styling issue

4cfc20f

remove unneeded local state for focus mode

7e4bc4f

added new cases for params flag

70cc2bd

solved problem with missing arrowhead when triggering between values …

73560b1

…of newParamsFlag

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

3b19a52

…into main

removed newparams flag settings

f5c8318

fixed edges issue

2016250

fix edge case for sidebar for disabled status of input output nodes

07741c7

added tests, fixed failing tests and refactored css class

7df247f

added all tests for newly added selectors, actions and functionality …

e7bf871

…within node-list-items

studioswong requested review from bru5, limdauto, rashidakanchwala and yetudada August 5, 2021 00:46

studioswong added 3 commits August 5, 2021 13:03

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

9e37956

…into main

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

4779a5d

…into main

Merge branch 'main' into feature/focus-mode-2.0-merged

ef358ab

rashidakanchwala reviewed Aug 9, 2021

View reviewed changes

fixed typo

60ea57a

limdauto approved these changes Aug 9, 2021

View reviewed changes

rashidakanchwala approved these changes Aug 10, 2021

View reviewed changes

bru5 reviewed Aug 10, 2021

View reviewed changes

studioswong added 2 commits August 10, 2021 18:13

refactor due to PR suggestions

6f545eb

further updates per PR comments

5a3d526

studioswong merged commit 361964b into main Aug 10, 2021

studioswong deleted the feature/focus-mode-2.0-merged branch August 10, 2021 19:29

studioswong commented Aug 20, 2021

View reviewed changes

snyk-bot mentioned this pull request Nov 23, 2021

[Snyk] Upgrade reselect from 4.1.0 to 4.1.1 #642

Closed

	it('includes input output ndoes related to a modular pipeline in the returned object', () => {
	it('includes input output nodes related to a modular pipeline in the returned object', () => {

		@@ -84,7 +100,11 @@ const NodeListProvider = ({
		if (isGroupType(item.type) \|\| isModularPipelineType(item.type)) {

[KED-2770] - Expose related dataset nodes for focus mode flowchart #530

[KED-2770] - Expose related dataset nodes for focus mode flowchart #530

Conversation

studioswong commented Aug 5, 2021

Description

Development notes

QA notes

Checklist

Legal notice

Choose a reason for hiding this comment

limdauto Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limdauto left a comment

Choose a reason for hiding this comment

limdauto Aug 9, 2021 • edited Loading

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

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

studioswong Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

rashidakanchwala left a comment

Choose a reason for hiding this comment

bru5 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

studioswong Aug 10, 2021 • edited Loading

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

studioswong Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

studioswong commented Aug 10, 2021

studioswong left a comment • edited Loading

Choose a reason for hiding this comment

limdauto Aug 9, 2021 •

edited

Loading

limdauto Aug 9, 2021 •

edited

Loading

studioswong Aug 10, 2021 •

edited

Loading

studioswong Aug 10, 2021 •

edited

Loading

studioswong Aug 10, 2021 •

edited

Loading

studioswong left a comment •

edited

Loading