kedro-org / kedro-viz

Visualise your Kedro data and machine-learning pipelines and track your experiments.
https://demo.kedro.org
Apache License 2.0
680 stars 113 forks source link

Introduce `behaviour` prop object with `reFocus` prop #2161

Closed jitu5 closed 2 weeks ago

jitu5 commented 3 weeks ago

Description

Resolves https://github.com/kedro-org/vscode-kedro/issues/126

This PR Introduce behaviour prop object with reFocus prop. When behaviour.reFocus is set to false in embedded mode and when user clicks on node it will not re-focus.

behaviour: { 
     reFocus: false, // default is true
}

Development notes

FlowChart component:

State management updates:

QA Notes:

You can test new options prop in src/components/container.js as <App /> is the entry point or top level component for a standalone use case.

const Container = () => (
  <>
    <App
      options={{
        display: {
          globalNavigation: false,
          sidebar: false,
          metadataPanel: false,
        },
        behaviour: {
          reFocus: false,
        },
      }}
      data={getPipelineData()}
    />
  </>
);

Checklist

ravi-kumar-pilla commented 3 weeks ago

Hi @jitu5 , Thanks for the PR. I left some comments around naming but the functionality looks good. Thank you

jitu5 commented 3 weeks ago

I tested it and it's all working fine for me. I agree with @ravi-kumar-pilla on the modeFocus name as it can be a bit confusing when you read it. Perhaps something about behaviour or interaction would work well in this context @jitu5 ?

@Huongg

modeFocus

You mean modeOptions ? To avoid confusion we can think reFocus is one of the behaviours.

 behaviour: { 
     reFocus: false, // default is true
}

@ravi-kumar-pilla @rashidakanchwala wdyt ?

ravi-kumar-pilla commented 3 weeks ago
behaviour: { 
    reFocus: false, // default is true
}

Since this is a user facing key (though we document it) this should be very clear on what it does. For me behavior seems very generic. flowchartBehavior seems more appropriate. Considering if we expand on other behaviors, having a behavior key might introduce additional nesting. But I am fine with either. Thank you

rashidakanchwala commented 3 weeks ago
behaviour: { 
    reFocus: false, // default is true
}

Since this is a user facing key (though we document it) this should be very clear on what it does. For me behavior seems very generic. flowchartBehavior seems more appropriate. Considering if we expand on other behaviors, having a behavior key might introduce additional nesting. But I am fine with either. Thank you

nesting won't be a problem as we do deepmerge anyway right, @jitu5 ?

jitu5 commented 3 weeks ago
behaviour: { 
    reFocus: false, // default is true
}

Since this is a user facing key (though we document it) this should be very clear on what it does. For me behavior seems very generic. flowchartBehavior seems more appropriate. Considering if we expand on other behaviors, having a behavior key might introduce additional nesting. But I am fine with either. Thank you

nesting won't be a problem as we do deepmerge anyway right, @jitu5 ?

@rashidakanchwala Yes, we do deepmerge.