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

Dropdown Select Widget #17

Merged
merged 3 commits into from
Jan 24, 2021
Merged

Conversation

tirix
Copy link
Contributor

@tirix tirix commented Jan 24, 2021

Adding two new widgets, a list selection widget, and a dropdown selection widget, as shown in zulip post.

Copy link
Collaborator

@rjwittams rjwittams left a comment

Choose a reason for hiding this comment

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

Few comments but fine for nursery... can see where it ends up.

/// Given a vector of `(label_text, enum_variant)` tuples, create a dropdown select widget
/// This is exactly the same interface as `Radio` so that both can be used interchangably,
/// with dropdown taking less space in the UI.
pub fn new(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels like 'new' is the wrong name here because we aren't returning an instance of DropDownSelect (which seems to be 'just' a controller?)
I think we are missing some stuff to be able to conveniently declare this kind of composite widget. (Tabs and druid-table go to great lengths to wrap their internals up in a widget rather than return a bare Scope....) . Maybe some macro but I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could easily rename this build or build_widget. Would it be better?
I'll check how Tabs and druid-table handle this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure the way that they do it is necessarily something to spread around too much at the moment (which boils down to another wrapper just to hide their contents) - maybe this is a pattern we want to make easier in the end. In my mind it should be nearly as easy as declaring a component in a reactive web framework like Vue - modulo Rust vs JS/TS issues. Definitely needs a bit of thought....

I think build_widget might be a decent idea. I don't think I currently have a decent idea about scoping, but it does seem slightly weird that it is within a struct that happens to be used as a Controller deeper in the graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the best solution for now is to split ListSelect into the empty shell doing the build_widget on one side, and a ListSelectController handling the up/down arrows for changing selection on the other side. That's both an easy and proper way to handle this. I'll also change the comment to ListSelect so it does not claim to be a widget, but rather to build one. Same for DropdownSelect which has the same architecture.
I'll include that in my next PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to me, the controller is really just an implementation detail. I think the "right" patterns will take a while to emerge...

env: &Env,
) {
if let LifeCycle::HotChanged(false) = event {
ctx.submit_command(COLLAPSE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will send the command to the window, which would mean multiple ones in the same window would screw each other up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, you are right in principle, but actually, the dropdown is supposed to collapse as soon as the mouse exits it, so any other select dropdown in the same window would already be collapsed and the fact that they receive an additional "collapse" command would not change anything...


fn from_label(label: Label<T>) -> DropdownButton<T> {
DropdownButton {
wedge: WidgetPod::new(Wedge::new()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels like it would make sense to lens this to the tuple you want rather than manually doing it in each method?
Ah, this one would make it easier linebender/druid#1540 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but unfortunately it's not merged yet...
I've chosen to simplify the Wedge widget to have it work with boolean, this removes the (somewhat ugly) tuple.

If I was to lens it, the concrete Wedge type in the definition of the struct DropdownButton would get replaced by something like LensWarp<bool, DropdownState<T>, Lens<bool, DropdownState<T>>, Wedge>, that's quite long, is there a better choice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if there is a better way at the moment really. It is either that or boxing I think.
I do sometimes put in type aliases for these composed things, which can at least make the struct look less noisy


// Is "Chevron" a better name?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably

Copy link
Collaborator

@derekdreery derekdreery left a comment

Choose a reason for hiding this comment

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

Awesome let's get it in and see how peole use them.

@derekdreery derekdreery merged commit 916a41d into linebender:master Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants