-
Notifications
You must be signed in to change notification settings - Fork 323
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
Select node on any edit #9536
Select node on any edit #9536
Conversation
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.
The general solution seems fine, but I think the zIndex
being optional has potential to cause serious issues later, so let's address that before merging.
@@ -517,7 +521,9 @@ export interface NodeDataFromMetadata { | |||
vis: Opt<VisualizationMetadata> | |||
} | |||
|
|||
export interface Node extends NodeDataFromAst, NodeDataFromMetadata {} | |||
export interface Node extends NodeDataFromAst, NodeDataFromMetadata { | |||
zIndex?: number |
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 assume that right now the default state here is undefined
(or rather, as missing field). I think it will cause less issues down the line if we make this field required and explicitly set the zIndex
for each created node. That way newly added nodes will always be rendered on top, instead of being under all nodes that happened to have zIndex
set to some value.
20a0713
to
6ea43ac
Compare
Pull Request Description
Fixes #7478
Screencast.from.2024-03-25.15-44-50.webm
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.