Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flowchart Orientation #2251

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
13 changes: 13 additions & 0 deletions src/actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,19 @@ export function toggleLayers(visible) {
};
}

export const TOGGLE_ORIENTATION = 'TOGGLE_ORIENTATION';

/**
* Toggle whether to show horizontal or vertical orientation
* @param {string} orientation The orientation to set to vertical by default
*/
export function toggleOrientation(orientation) {
return {
type: TOGGLE_ORIENTATION,
orientation,
};
}

export const TOGGLE_EXPAND_ALL_PIPELINES = 'TOGGLE_EXPAND_ALL_PIPELINES';

/**
Expand Down
4 changes: 4 additions & 0 deletions src/components/app/app.scss
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
--color-base-20: #{colors.$grey-300};
--color-black-10: #{colors.$white-200};
--color-border-line: #{colors.$white-500};
--color-arrow-primary: #{colors.$black-900};
--color-arrow-secondary: #{colors.$black-0};

// Experiment tracking colors below
--color-exp-tracking-bg: #{colors.$white-0};
Expand Down Expand Up @@ -67,6 +69,8 @@
--color-run-list-hover: #{colors.$slate-0};
--color-base-20: #{colors.$grey-800};
--color-black-10: #{colors.$slate-700};
--color-arrow-primary: #{colors.$white-0};
--color-arrow-secondary: #{colors.$black-300};

// Experiment tracking colors below
--color-exp-tracking-bg: #{colors.$slate-900};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@ import {
toggleSidebar,
toggleTextLabels,
toggleExpandAllPipelines,
toggleOrientation,
} from '../../actions';
import { toggleModularPipelinesVisibilityState } from '../../actions/modular-pipelines';
import IconButton from '../ui/icon-button';
import LabelIcon from '../icons/label';
import ExportIcon from '../icons/export';
import LayersIcon from '../icons/layers';
import LeftRightIcon from '../icons/left-right';
import TopBottomIcon from '../icons/top-bottom';
import PrimaryToolbar from '../primary-toolbar';
import { getVisibleLayerIDs } from '../../selectors/disabled';
import ExpandPipelinesIcon from '../icons/expand-pipelines';
Expand All @@ -35,6 +38,8 @@ export const FlowchartPrimaryToolbar = ({
visibleLayers,
expandedPipelines,
onToggleExpandAllPipelines,
orientation,
onToggleOrientation,
}) => {
const { toSetQueryParam } = useGeneratePathname();

Expand Down Expand Up @@ -97,6 +102,19 @@ export const FlowchartPrimaryToolbar = ({
onClick={() => onToggleExportModal(true)}
visible={display.exportBtn}
/>
<IconButton
ariaLabel="Change flowchart orientation"
className={'pipeline-menu-button--orientation'}
dataTest={'sidebar-flowchart-orientation-btn'}
icon={orientation === 'vertical' ? TopBottomIcon : LeftRightIcon}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be opposite ? Like when vertical we show LeftRightIcon to change, similar to expand/collapse pipelines ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a very good point. i am not sure; once @stephkaiser is back; let's ask her.

labelText="Change Orientation"
onClick={() =>
onToggleOrientation(
orientation === 'vertical' ? 'horizontal' : 'vertical'
)
}
visible={display.orientationBtn}
/>
</PrimaryToolbar>
</>
);
Expand All @@ -108,6 +126,7 @@ export const mapStateToProps = (state) => ({
visible: state.visible,
display: state.display,
visibleLayers: Boolean(getVisibleLayerIDs(state).length),
orientation: state.orientation,
expandedPipelines: state.expandAllPipelines,
});

Expand All @@ -128,6 +147,9 @@ export const mapDispatchToProps = (dispatch) => ({
dispatch(toggleExpandAllPipelines(isExpanded));
dispatch(toggleModularPipelinesVisibilityState(isExpanded));
},
onToggleOrientation: (value) => {
dispatch(toggleOrientation(value));
},
});

export default connect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jest.mock('../../utils/hooks/use-generate-pathname', () => ({
describe('PrimaryToolbar', () => {
it('renders without crashing', () => {
const wrapper = setup.mount(<ConnectedFlowchartPrimaryToolbar />);
expect(wrapper.find('.pipeline-icon-toolbar__button').length).toBe(5);
expect(wrapper.find('.pipeline-icon-toolbar__button').length).toBe(6);
});

it('hides all buttons (except menu button) when display prop is false for each of them', () => {
Expand All @@ -24,6 +24,7 @@ describe('PrimaryToolbar', () => {
layerBtn: false,
exportBtn: false,
expandPipelinesBtn: false,
orientationBtn: false,
};
const wrapper = setup.mount(<ConnectedFlowchartPrimaryToolbar />, {
options: { display },
Expand All @@ -38,7 +39,7 @@ describe('PrimaryToolbar', () => {
const wrapper = setup.mount(<ConnectedFlowchartPrimaryToolbar />, {
options: { display },
});
expect(wrapper.find('.pipeline-icon-toolbar__button').length).toBe(4);
expect(wrapper.find('.pipeline-icon-toolbar__button').length).toBe(5);
});

const functionCalls = [
Expand Down Expand Up @@ -74,6 +75,7 @@ describe('PrimaryToolbar', () => {
disableLayerBtn: expect.any(Boolean),
textLabels: expect.any(Boolean),
expandedPipelines: expect.any(Boolean),
orientation: expect.any(String),
visible: expect.objectContaining({
exportModal: expect.any(Boolean),
metadataModal: expect.any(Boolean),
Expand All @@ -84,6 +86,7 @@ describe('PrimaryToolbar', () => {
exportBtn: expect.any(Boolean),
labelBtn: expect.any(Boolean),
layerBtn: expect.any(Boolean),
orientationBtn: expect.any(Boolean),
expandPipelinesBtn: expect.any(Boolean),
}),
visibleLayers: expect.any(Boolean),
Expand Down
36 changes: 26 additions & 10 deletions src/components/flowchart/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,22 @@ export const drawLayers = function () {
* Render layer name labels
*/
export const drawLayerNames = function () {
const {
chartSize: { sidebarWidth = 0 },
layers,
} = this.props;
const { chartSize, layers, orientation } = this.props;

const layerNamePosition =
orientation === 'vertical' ? chartSize.sidebarWidth || 0 : 100 || 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be possible to add some comments on these calculations for future ref ? Thank you


const transformValue =
orientation === 'vertical'
? // In vertical mode, layer names are positioned along the X-axis at sidebarWidth
`translateX(${layerNamePosition}px)`
: // In horizontal mode, layer names are positioned at a fixed Y = 100px
`translateY(${layerNamePosition}px)`;

this.el.layerNameGroup
.transition('layer-names-sidebar-width')
.duration(this.DURATION)
.style('transform', `translateX(${sidebarWidth}px)`);
.style('transform', transformValue);

this.el.layerNames = this.el.layerNameGroup
.selectAll('.pipeline-layer-name')
Expand Down Expand Up @@ -126,12 +133,20 @@ const updateNodeRects = (nodeRects) =>
return node.height / 2;
});

const updateParameterRect = (nodeRects) =>
const updateParameterRect = (nodeRects, orientation) =>
nodeRects
.attr('width', 12)
.attr('height', 12)
.attr('x', (node) => (node.width + 20) / -2)
.attr('y', -6);
.attr('x', (node) =>
// Position parameter icon on the left side of the node in vertical mode
// Position it slightly inside the node in horizontal mode
orientation === 'vertical'
? (node.width + 20) / -2
: -(node.width / 2) + 10
)
// Center parameter icon vertically on the left side of the node (12px parameter icon height, so -6 for centering)
// Place parameter icon on top of the node (12px parameter icon height)
.attr('y', (node) => (orientation === 'vertical' ? -6 : -node.height + 12));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be possible to add some comments on these calculations for future ref ? Thank you


/**
* Render node icons and name labels
Expand All @@ -150,6 +165,7 @@ export const drawNodes = function (changed) {
focusMode,
hoveredFocusMode,
isSlicingPipelineApplied,
orientation,
} = this.props;
const {
from: slicedPipelineFromId,
Expand Down Expand Up @@ -223,7 +239,7 @@ export const drawNodes = function (changed) {
.append('rect')
.attr('class', 'pipeline-node__parameter-indicator')
.on('mouseover', this.handleParamsIndicatorMouseOver)
.call(updateParameterRect);
.call(updateParameterRect, orientation);

// Performance: use a single path per icon
enterNodes
Expand Down Expand Up @@ -344,7 +360,7 @@ export const drawNodes = function (changed) {
)
.transition('node-rect')
.duration((node) => (node.showText ? 200 : 600))
.call(updateParameterRect);
.call(updateParameterRect, orientation);

// Performance: icon transitions with CSS on GPU
allNodes
Expand Down
21 changes: 16 additions & 5 deletions src/components/flowchart/flowchart.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export class FlowChart extends Component {
this.updateChartSize();
}

if (changed('layers', 'chartSize')) {
if (changed('layers', 'chartSize', 'orientation')) {
drawLayers.call(this);
drawLayerNames.call(this);
}
Expand Down Expand Up @@ -364,8 +364,14 @@ export class FlowChart extends Component {
// Update layer label y positions
if (this.el.layerNames) {
this.el.layerNames.style('transform', (d) => {
const updateY = y + (d.y + d.height / 2) * scale;
return `translateY(${updateY}px)`;
if (this.props.orientation === 'vertical') {
const updateY = y + (d.y + d.height / 2) * scale;
return `translateY(${updateY}px)`; // Use translateY for vertical layout
} else {
// Horizontal orientation
const updateX = x + (d.x + d.width / 4) * scale;
return `translateX(${updateX}px)`; // Use translateX for horizontal layout
}
});
}

Expand Down Expand Up @@ -473,7 +479,8 @@ export class FlowChart extends Component {
* Zoom and scale to fit graph and any selected node in view
*/
resetView(preventZoom) {
const { chartSize, graphSize, clickedNode, nodes } = this.props;
const { chartSize, graphSize, clickedNode, nodes, orientation } =
this.props;
const { width: chartWidth, height: chartHeight } = chartSize;
const { width: graphWidth, height: graphHeight } = graphSize;

Expand All @@ -491,19 +498,22 @@ export class FlowChart extends Component {
: null;

// Find a transform that fits everything in view

const transform = viewTransformToFit({
offset,
focus,
viewWidth: chartWidth,
viewHeight: chartHeight,
objectWidth: graphWidth,
objectHeight: graphHeight,
minScaleX: 0.2,
sidebarWidth: chartSize.sidebarWidth,
minScaleX: 0.05,
minScaleFocus: this.props.visibleMetaSidebar
? this.props.chartZoom.scale
: 0.1,
focusOffset: 0,
preventZoom,
orientation,
});

// Detect first transform
Expand Down Expand Up @@ -997,6 +1007,7 @@ export const mapStateToProps = (state, ownProps) => ({
nodeSelected: getNodeSelected(state),
nodesWithInputParams: getNodesWithInputParams(state),
modularPipelineIds: state.modularPipeline.ids,
orientation: state.orientation,
inputOutputDataNodes: getInputOutputNodesForFocusedModularPipeline(state),
inputOutputDataEdges: getInputOutputDataEdges(state),
visibleGraph: state.visible.graph,
Expand Down
Loading
Loading