-
Notifications
You must be signed in to change notification settings - Fork 113
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
Conversation
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
Signed-off-by: Jitendra Gundaniya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thanks
src/reducers/index.js
Outdated
@@ -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({}), |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravi-kumar-pilla
Please refer kedro-org/vscode-kedro#126 (comment)
There was a problem hiding this comment.
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?
Hi @jitu5 , Thanks for the PR. I left some comments around naming but the functionality looks good. Thank you |
Signed-off-by: Jitendra Gundaniya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
@@ -220,22 +220,28 @@ export class FlowChart extends Component { | |||
|
|||
if (changed('edges', 'nodes', 'layers', 'chartSize', 'clickedNode')) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
You mean
|
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. |
nesting won't be a problem as we do deepmerge anyway right, @jitu5 ? |
@rashidakanchwala Yes, we do deepmerge. |
Signed-off-by: Jitendra Gundaniya <[email protected]>
modeOptions
prop object with reFocus
propbehaviour
prop object with reFocus
prop
Signed-off-by: Jitendra Gundaniya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well !! Thank you @jitu5
Description
Resolves kedro-org/vscode-kedro#126
This PR Introduce
behaviour
prop object withreFocus
prop. Whenbehaviour.reFocus
is set tofalse
in embedded mode and when user clicks on node it will not re-focus.Development notes
FlowChart
component:src/components/flowchart/flowchart.js
: Refactored the conditions for redrawing nodes and edges to prevent unnecessary zooming when the metadata sidebar or code panel visibility changes, or when the clicked node changes without node refocusing.State management updates:
src/reducers/index.js
: Added a new reducer forbehaviour
to manage thereFocus
state.src/store/initial-state.js
: Initialized thebehaviour
state with a defaultreFocus
value oftrue
.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.Checklist
RELEASE.md
file