From 8b7c8d87a8eea6fab45fe0d6574c0fd7c594e3a1 Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Tue, 13 Aug 2024 14:57:11 +0100 Subject: [PATCH 1/9] change order only when store updated Signed-off-by: Sajid Alam --- src/components/flowchart/flowchart.js | 67 ++++++++++++--------------- 1 file changed, 30 insertions(+), 37 deletions(-) diff --git a/src/components/flowchart/flowchart.js b/src/components/flowchart/flowchart.js index b07fd08fe1..4ec88990bf 100644 --- a/src/components/flowchart/flowchart.js +++ b/src/components/flowchart/flowchart.js @@ -552,66 +552,59 @@ export class FlowChart extends Component { }; /** - * Determines the correct order of nodes based on their positions, - * but keeps the user's original selection as the visual 'from' node. - * @param {string} userSelectedFromNodeId - User's initially selected 'from' node ID. - * @param {string} toNodeId - The current 'to' node ID. - * @returns {Object} - Object containing userVisualFromNodeId and adjustedFromNodeId, newToNodeId + * Determines the correct order of nodes based on their positions. + * @param {string} fromNodeId - 'From' node ID. + * @param {string} toNodeId - 'To' node ID. + * @returns {Object} - Object containing updatedFromNodeId and updatedToNodeId. */ - determineNodesOrder = (userSelectedFromNodeId, toNodeId) => { + determineNodesOrder = (fromNodeId, toNodeId) => { // Get bounding client rects of nodes - const fromNodeElement = document.querySelector( - `[data-id="${userSelectedFromNodeId}"]` - ); + const fromNodeElement = document.querySelector(`[data-id="${fromNodeId}"]`); const toNodeElement = document.querySelector(`[data-id="${toNodeId}"]`); if (!fromNodeElement || !toNodeElement) { return { - userVisualFromNodeId: null, - adjustedFromNodeId: null, - newToNodeId: null, - }; // If any element is missing, return nulls + updatedFromNodeId: null, + updatedToNodeId: null, + }; } const fromNodeRect = fromNodeElement.getBoundingClientRect(); const toNodeRect = toNodeElement.getBoundingClientRect(); - // Reorder the nodes based on their Y-coordinate - const [adjustedFromNodeId, newToNodeId] = - fromNodeRect.y < toNodeRect.y - ? [userSelectedFromNodeId, toNodeId] // Keep order - : [toNodeId, userSelectedFromNodeId]; // Swap if needed - - return { - userVisualFromNodeId: userSelectedFromNodeId, // Keep user's selection visually as 'from' - adjustedFromNodeId, - newToNodeId, - }; + // Reorder based on their Y-coordinate + return fromNodeRect.y < toNodeRect.y + ? { updatedFromNodeId: fromNodeId, updatedToNodeId: toNodeId } + : { updatedFromNodeId: toNodeId, updatedToNodeId: fromNodeId }; }; handleShiftClick = (node) => { // Close meta data panel this.props.onLoadNodeData(null); - const { from: userSelectedFromNodeId, range } = - this.state.slicedPipelineState; + const { from: fromNodeIdState, range } = this.state.slicedPipelineState; - const fromNodeId = userSelectedFromNodeId || node.id; // Keep existing 'from' node or set the current one if not set + // Track user's selection directly in the state + const fromNodeId = fromNodeIdState || node.id; const toNodeId = node.id; - const { userVisualFromNodeId, adjustedFromNodeId, newToNodeId } = - this.determineNodesOrder(fromNodeId, toNodeId); + // Update the state to reflect the user's selections + this.updateSlicedPipelineState(fromNodeId, toNodeId, range); - if (!adjustedFromNodeId || !newToNodeId) { - return; // Exit if node order couldn't be determined - } + // Reorder nodes internally for slicing + const { updatedFromNodeId, updatedToNodeId } = this.determineNodesOrder( + fromNodeId, + toNodeId + ); - // Visually keep the 'from' node as the user's selection, but adjust internally based on Y-coordinate - this.updateSlicedPipelineState(userVisualFromNodeId, newToNodeId, range); + // Slice the pipeline based on the determined order, or fallback to original order if undefined + if (updatedFromNodeId && updatedToNodeId) { + this.props.onSlicePipeline(updatedFromNodeId, updatedToNodeId); + } else { + this.props.onSlicePipeline(fromNodeId, toNodeId); + } - this.props.onSlicePipeline(adjustedFromNodeId, newToNodeId); this.props.onApplySlice(false); - - this.setState({ showSlicingNotification: false }); // Hide notification after selecting the second node + this.setState({ showSlicingNotification: false }); // Hide notification }; /** From c890945111157b4df555320ebc7d59eda204e09f Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Tue, 13 Aug 2024 15:23:57 +0100 Subject: [PATCH 2/9] update tests Signed-off-by: Sajid Alam --- .../sliced-pipeline-action-bar.test.js | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js b/src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js index f615dd39d0..a2a3eb4bca 100644 --- a/src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js +++ b/src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js @@ -1,6 +1,5 @@ -// sliced-pipeline-action-bar.test.js import React from 'react'; -import { render, screen } from '@testing-library/react'; +import { render } from '@testing-library/react'; import { SlicedPipelineActionBar } from './sliced-pipeline-action-bar'; describe('SlicedPipelineActionBar', () => { @@ -10,7 +9,7 @@ describe('SlicedPipelineActionBar', () => { firstChild: { getBoundingClientRect: () => ({ width: 100 }) }, }, }; - render( + const { container } = render( { ref={ref} /> ); - const notification = screen.getByText( - /Hold Shift \+ Click on another node to slice pipeline/i + + // Check for the presence of the notification class + const notificationElement = container.querySelector( + '.sliced-pipeline-action-bar--info' + ); + expect(notificationElement).toBeInTheDocument(); + expect(notificationElement.textContent).toContain( + 'Hold Shift + Click on another node to slice pipeline' ); - expect(notification).toBeInTheDocument(); }); it('displays "Slice" button when slicedPipeline is not empty', () => { @@ -32,7 +36,7 @@ describe('SlicedPipelineActionBar', () => { firstChild: { getBoundingClientRect: () => ({ width: 100 }) }, }, }; - render( + const { container } = render( { ref={ref} /> ); - const sliceButton = screen.getByRole('button', { name: /slice/i }); + + const ctaElement = container.querySelector( + '.sliced-pipeline-action-bar--cta' + ); + expect(ctaElement).toBeInTheDocument(); + + const sliceButton = ctaElement.querySelector('button'); expect(sliceButton).toBeInTheDocument(); + expect(sliceButton.textContent).toBe('Slice'); }); }); From 6324e1d9bbde559b2c42111b720b9891e93108f0 Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Wed, 14 Aug 2024 14:48:23 +0100 Subject: [PATCH 3/9] changes based on review Signed-off-by: Sajid Alam --- src/components/flowchart/flowchart.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/flowchart/flowchart.js b/src/components/flowchart/flowchart.js index 4ec88990bf..9e6b424500 100644 --- a/src/components/flowchart/flowchart.js +++ b/src/components/flowchart/flowchart.js @@ -140,8 +140,7 @@ export class FlowChart extends Component { if (!this.props.clickedNode && prevProps.clickedNode) { if (!this.props.slicedPipeline.length) { // Ensure this only runs when not in a slicing operation - this.updateSlicedPipelineState(null, null, []); - this.setState({ showSlicingNotification: false }); // Hide notification when clicking away + this.setState({ showSlicingNotification: false }); } } } From 2e62a58cc0a84ae9e86b727338161825ab520926 Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Thu, 15 Aug 2024 11:31:17 +0100 Subject: [PATCH 4/9] Set showSlicingNotification to true if a single node is clicked Signed-off-by: Sajid Alam --- src/components/flowchart/flowchart.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/flowchart/flowchart.js b/src/components/flowchart/flowchart.js index 9e6b424500..50841f5830 100644 --- a/src/components/flowchart/flowchart.js +++ b/src/components/flowchart/flowchart.js @@ -537,7 +537,7 @@ export class FlowChart extends Component { // then on a single node click, it should reset the sliced pipeline state if (from !== null && to !== null) { this.updateSlicedPipelineState(null, null, []); - this.setState({ showSlicingNotification: false }); // Hide notification + this.setState({ showSlicingNotification: true }); // Show notification } else { // Else, set the first node as the 'from' node based on current state // we need this so that if user hold shift and click on a second node, From 4279a8222c0bce8d21fd5c3bed70ca9021c17f7d Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Thu, 15 Aug 2024 15:33:00 +0100 Subject: [PATCH 5/9] Update flowchart.js Signed-off-by: Sajid Alam --- src/components/flowchart/flowchart.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/flowchart/flowchart.js b/src/components/flowchart/flowchart.js index 50841f5830..e02b85e1dc 100644 --- a/src/components/flowchart/flowchart.js +++ b/src/components/flowchart/flowchart.js @@ -536,7 +536,7 @@ export class FlowChart extends Component { // if both "from" and "to" are defined // then on a single node click, it should reset the sliced pipeline state if (from !== null && to !== null) { - this.updateSlicedPipelineState(null, null, []); + this.updateSlicedPipelineState(id, null, []); this.setState({ showSlicingNotification: true }); // Show notification } else { // Else, set the first node as the 'from' node based on current state From f80684c84bb6d6fe3ba76a0e2cab171507aaf653 Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Thu, 15 Aug 2024 18:59:54 +0100 Subject: [PATCH 6/9] enable notification when single node is clicked Signed-off-by: Sajid Alam --- src/components/flowchart/flowchart.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/flowchart/flowchart.js b/src/components/flowchart/flowchart.js index 8e9114a0cf..a32b77d84f 100644 --- a/src/components/flowchart/flowchart.js +++ b/src/components/flowchart/flowchart.js @@ -558,7 +558,7 @@ export class FlowChart extends Component { // Also, prepare the "from" node for the next slicing action this.updateSlicedPipelineState(id, null, []); // Hide notification - this.setState({ showSlicingNotification: false }); + this.setState({ showSlicingNotification: true }); } }; From 87cdae4c54852c8810c8811cd31db8ed07d1c19c Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Thu, 15 Aug 2024 19:02:58 +0100 Subject: [PATCH 7/9] Delete sliced-pipeline-action-bar.test.js Signed-off-by: Sajid Alam --- .../sliced-pipeline-action-bar.test.js | 59 ------------------- 1 file changed, 59 deletions(-) delete mode 100644 src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js diff --git a/src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js b/src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js deleted file mode 100644 index a2a3eb4bca..0000000000 --- a/src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.test.js +++ /dev/null @@ -1,59 +0,0 @@ -import React from 'react'; -import { render } from '@testing-library/react'; -import { SlicedPipelineActionBar } from './sliced-pipeline-action-bar'; - -describe('SlicedPipelineActionBar', () => { - it('displays notification message when notification prop is true', () => { - const ref = { - current: { - firstChild: { getBoundingClientRect: () => ({ width: 100 }) }, - }, - }; - const { container } = render( - - ); - - // Check for the presence of the notification class - const notificationElement = container.querySelector( - '.sliced-pipeline-action-bar--info' - ); - expect(notificationElement).toBeInTheDocument(); - expect(notificationElement.textContent).toContain( - 'Hold Shift + Click on another node to slice pipeline' - ); - }); - - it('displays "Slice" button when slicedPipeline is not empty', () => { - const ref = { - current: { - firstChild: { getBoundingClientRect: () => ({ width: 100 }) }, - }, - }; - const { container } = render( - - ); - - const ctaElement = container.querySelector( - '.sliced-pipeline-action-bar--cta' - ); - expect(ctaElement).toBeInTheDocument(); - - const sliceButton = ctaElement.querySelector('button'); - expect(sliceButton).toBeInTheDocument(); - expect(sliceButton.textContent).toBe('Slice'); - }); -}); From 0ec2d68e883dea2dc1195fa6008984bcfacf3744 Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Thu, 15 Aug 2024 19:06:26 +0100 Subject: [PATCH 8/9] add comments Signed-off-by: Sajid Alam --- src/components/flowchart/flowchart.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/flowchart/flowchart.js b/src/components/flowchart/flowchart.js index a32b77d84f..359d0c8860 100644 --- a/src/components/flowchart/flowchart.js +++ b/src/components/flowchart/flowchart.js @@ -592,6 +592,7 @@ export class FlowChart extends Component { handleMultipleNodesClick = (node) => { // Close meta data panel this.props.onLoadNodeData(null); + const { from: fromNodeIdState, range } = this.state.slicedPipelineState; const fromNodeId = fromNodeIdState || node.id; @@ -604,6 +605,8 @@ export class FlowChart extends Component { toNodeId ); + // Slice the pipeline based on the determined node order + // If the order could not be determined, use the original selection if (updatedFromNodeId && updatedToNodeId) { this.props.onSlicePipeline(updatedFromNodeId, updatedToNodeId); } else { From 0275ed66743697d7b280ddc41712ff2357ac04fa Mon Sep 17 00:00:00 2001 From: Sajid Alam Date: Thu, 15 Aug 2024 19:15:58 +0100 Subject: [PATCH 9/9] merge Signed-off-by: Sajid Alam --- src/components/flowchart/flowchart.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/components/flowchart/flowchart.js b/src/components/flowchart/flowchart.js index 359d0c8860..f9bcd1eee2 100644 --- a/src/components/flowchart/flowchart.js +++ b/src/components/flowchart/flowchart.js @@ -139,11 +139,11 @@ export class FlowChart extends Component { } // Hide slicing notification if metadata panel is closed using button - if (!this.props.clickedNode && prevProps.clickedNode) { - if (!this.props.slicedPipeline.length) { - // Ensure this only runs when not in a slicing operation - this.setState({ showSlicingNotification: false }); - } + if ( + this.props.clickedNode !== prevProps.clickedNode && + !this.props.clickedNode + ) { + this.setState({ showSlicingNotification: false }); } } @@ -577,7 +577,7 @@ export class FlowChart extends Component { return { updatedFromNodeId: null, updatedToNodeId: null, - }; + }; // If any element is missing, return nulls } const fromNodeRect = fromNodeElement.getBoundingClientRect();