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

Introduce behaviour prop object with reFocus prop #2161

Merged
merged 12 commits into from
Nov 4, 2024
5 changes: 5 additions & 0 deletions README.npm.md
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@
tag: {
enabled: {companies: true}
},
modeOptions: {
reFocus: true,
},
theme: "dark"
}}
/>
Expand All @@ -161,6 +164,8 @@
| `sidebar` | boolean | true | Show/Hide Sidebar and action toolbar |
| `zoomToolbar` | boolean | true | Show/Hide zoom-in, zoom-out and zoom reset buttons together |
| options.expandAllPipelines | boolean | false | Expand/Collapse Modular pipelines on first load |
| options.modeOptions | | | |
| `reFocus` | boolean | true | Enable/Disable node re-focus on click

Check warning on line 168 in README.npm.md

View workflow job for this annotation

GitHub Actions / vale

[vale] README.npm.md#L168

[Kedro-viz.Spellings] Did you really mean 'boolean'?
Raw output
{"message": "[Kedro-viz.Spellings] Did you really mean 'boolean'?", "location": {"path": "README.npm.md", "range": {"start": {"line": 168, "column": 21}}}, "severity": "WARNING"}
| options.nodeType | `{disabled: {parameters: boolean,task: boolean,data: boolean}}` | `{disabled: {parameters: true,task: false,data: false}}` | Configuration for node type options |
| options.tag | `{enabled: {<tagName>: boolean}}` | - | Configuration for tag options |
| options.theme | string | dark | select `Kedro-Viz` theme : dark/light |
Expand Down
1 change: 1 addition & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Please follow the established format:
## Major features and improvements

- Update Kedro-Viz telemetry for opt-out model (#2022)
- Introduce `modeOptions` prop object with `reFocus` prop (#2161)

## Bug fixes and other changes

Expand Down
6 changes: 6 additions & 0 deletions src/components/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@ App.propTypes = {
tag: PropTypes.shape({
enabled: PropTypes.objectOf(PropTypes.bool),
}),
/**
* Whether to re-focus the graph when a node is clicked
*/
modeOptions: PropTypes.shape({
reFocus: PropTypes.bool,
}),
/**
* Override the default enabled/disabled node types
*/
Expand Down
29 changes: 18 additions & 11 deletions src/components/flowchart/flowchart.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,22 +220,28 @@ export class FlowChart extends Component {

if (changed('edges', 'nodes', 'layers', 'chartSize', 'clickedNode')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, it looks like this section of code is for different beahviour of the stand-alone app. Perhaps having a comment here could help clarify the purpose, so we'll have better context if we refactor this component in the future. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Huongg Not exactly, those condition is also being used in normal mode, but yes refactoring flowchart.js component will really help overall.

// Don't zoom out when the metadata or code panels are opened or closed
ravi-kumar-pilla marked this conversation as resolved.
Show resolved Hide resolved
if (prevProps.visibleMetaSidebar !== this.props.visibleMetaSidebar) {
const metaSidebarChanged =
jitu5 marked this conversation as resolved.
Show resolved Hide resolved
prevProps.visibleMetaSidebar !== this.props.visibleMetaSidebar;

const codeChangedWithoutMetaSidebar =
jitu5 marked this conversation as resolved.
Show resolved Hide resolved
prevProps.visibleCode !== this.props.visibleCode &&
!this.props.visibleMetaSidebar;

// Don't zoom out when the clicked node changes and the nodeReFocus is disabled
const clickedNodeChangedWithoutReFocus =
prevProps.clickedNode !== this.props.clickedNode &&
!this.props.nodeReFocus;

if (
metaSidebarChanged ||
codeChangedWithoutMetaSidebar ||
clickedNodeChangedWithoutReFocus
) {
drawNodes.call(this, changed);
drawEdges.call(this, changed);

return;
}

if (prevProps.visibleCode !== this.props.visibleCode) {
if (!this.props.visibleMetaSidebar) {
drawNodes.call(this, changed);
drawEdges.call(this, changed);

return;
}
}

this.resetView(preventZoom);
} else {
this.onChartZoomChanged(chartZoom);
Expand Down Expand Up @@ -1000,6 +1006,7 @@ export const mapStateToProps = (state, ownProps) => ({
slicedPipeline: getSlicedPipeline(state),
isSlicingPipelineApplied: state.slice.apply,
visibleSlicing: state.visible.slicing,
nodeReFocus: state.modeOptions.reFocus,
runCommand: getRunCommand(state),
...ownProps,
});
Expand Down
1 change: 1 addition & 0 deletions src/components/flowchart/flowchart.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ describe('FlowChart', () => {
runCommand: expect.any(Object),
modularPipelineIds: expect.any(Object),
visibleSlicing: expect.any(Boolean),
nodeReFocus: expect.any(Boolean),
};
expect(mapStateToProps(mockState.spaceflights)).toEqual(expectedResult);
});
Expand Down
1 change: 1 addition & 0 deletions src/reducers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ const combinedReducer = combineReducers({
// These props don't have any actions associated with them
display: createReducer(null),
dataSource: createReducer(null),
modeOptions: createReducer({}),
Copy link
Contributor

Choose a reason for hiding this comment

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

[Nit] modeOptions inside options seems a bit confusing to me. Was there any consideration of having reFocus inside display ? If not, was there any reason to separate this ?
What does mode mean here ?

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla Oct 30, 2024

Choose a reason for hiding this comment

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

if we expand on how flowchart behaves, something like flowchartBehavior or something along those lines might give a better idea of what the key value is. For me modeOptions does not seem fitting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

hey since the stand-alone version primarily covers the flowchart, we might not need to specify 'flowchart.' Maybe something more general like behaviour, interaction, or interactionMode would work?

edge: createReducer({}),
// These props have very simple non-nested actions
chartSize: createReducer({}, UPDATE_CHART_SIZE, 'chartSize'),
Expand Down
3 changes: 3 additions & 0 deletions src/store/initial-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ export const createInitialState = () => ({
zoomToolbar: true,
metadataPanel: true,
},
modeOptions: {
reFocus: true,
},
zoom: {},
runsMetadata: {},
});
Expand Down
Loading