-
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
Proper dropdown active area and arrow placement #7561
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.
I'm not much familiar with the code, but after reading it and the docs carefully I even understood most of the things there! Nice job with it. A few minor nitpicks.
/// Declare whether this widget should be considered for creation despite a config override | ||
/// being present. Widgets that declare `true` here should always create a child widget on | ||
/// the same span-tree node, so that the override can eventually be applied to it. Only widgets | ||
/// declared before the widget selected by override will take priority over it. | ||
const QUERY_ON_OVERRIDE: bool = false; |
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 don't understand the last sentence. They will take priority over what? What are the consequences of taking priority?
Also, the working of the second sentence is also confusing. I suggest to slightly rewrite it as follows:
Widgets that declare `true` here must create a child widget on the same span-tree node, so that the override can be applied to this child instead.
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've added more docs explaining the implicit ordering and where it is defined.
app/gui/view/graph-editor/src/component/node/input/widget/separator.rs
Outdated
Show resolved
Hide resolved
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.
Shouldn't we also modify the Widget Configuration
section in the module documentation, to mention this QUERY_ON_OVERRIDE
const?
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.
Indeed, I've added a section there. Also changed the const name to PRIORITY_OVER_OVERRIDE
, since the old one didn't really make any sense. The query
word in never otherwise used in this context.
QA: 🟢 |
Pull Request Description
Fixes #7423
Refactored widget matching algorithm to allow creating wrapper widgets even in cases where the widget config override is present. That allowed the widgets to be reordered, such that the argument name ends up being inside the dropdown widget. That way clicking it opens the dropdown.
Added explicit manual layout for the dropdown arrow position. Now it is positioned on the center of a selected appropriate child widget. For prefix chains, the leftmost part of the prefix application (the method or constructor) is selected.
dropdown-placement.mp4
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
.