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

implement vertical slider orientation #875

Merged
merged 4 commits into from
Nov 13, 2021
Merged

implement vertical slider orientation #875

merged 4 commits into from
Nov 13, 2021

Conversation

B-Reif
Copy link
Contributor

@B-Reif B-Reif commented Nov 4, 2021

This PR implements a vertical orientation for Slider. Closes #589.

What's new

  • Sliders demo now includes a radio button toggle for orientation
  • New enum SliderOrientation with variants Horizontal and Vertical
  • New mutator function .orientation to set the slider orientation
  • New constructor functions, new_vertical and from_get_set_vertical to instantiate with vertical orientation.
  • Vertical Sliders use alternate keycodes for increment/decrement (up/down instead of right/left).

What's changed

  • Replaces horizontal-specific Slider logic with some functions matching on orientation instead. Some functions and parameters are renamed (e.g. x_range -> position_range).
  • Some helper functions are now methods on Slider which match on the Slider's orientation.

Alternatives

  • If API breakage is ok, we could add an orientation argument to existing constructor functions (instead of creating new constructor functions).
  • We could expose only the .orientation mutator function (instead of changing or creating constructor functions).
  • We could expose a vertical slider as a different widget. The logic is similar enough that I think the sharing here is appropriate.

@emilk
Copy link
Owner

emilk commented Nov 6, 2021

I've skimmed through it, and overall it looks good! Will review properly when I find the time.

egui/src/widgets/slider.rs Outdated Show resolved Hide resolved
egui/src/widgets/slider.rs Outdated Show resolved Hide resolved
egui/src/widgets/slider.rs Show resolved Hide resolved
egui/src/widgets/slider.rs Outdated Show resolved Hide resolved
@emilk
Copy link
Owner

emilk commented Nov 7, 2021

Screen Shot 2021-11-07 at 21 05 12

This looks a bit bad to me. I guess most people who use a vertical slider won't want the label text (no shown above), but the slider value would be very nice to show, and should be below the vertical slider.

@B-Reif
Copy link
Contributor Author

B-Reif commented Nov 9, 2021

Ok, made review changes and moved the value below the slider. Also added a new method add_contents to invoke inside of either ui.horizontal or ui.vertical respectively.

image

This implementation will arrange the slider, value, and then label in a vertical layout. We could further group the value and label onto one horizontal line.

@B-Reif
Copy link
Contributor Author

B-Reif commented Nov 9, 2021

I'm having some issues with the keyboard step functionality of Slider, but I experience them on the current master branch also. These could probably be resolved in a separate PR.

@B-Reif B-Reif requested a review from emilk November 12, 2021 21:12
ui.label("Slider orientation:");
ui.radio_value(vertical, false, "Horizontal");
ui.radio_value(vertical, true, "Vertical");
});
Copy link
Owner

Choose a reason for hiding this comment

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

The f32, usize etc are also possible line below belongs together with the i32/f64 switch above

@emilk emilk merged commit 491739b into emilk:master Nov 13, 2021
emilk added a commit that referenced this pull request Nov 13, 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.

Vertical Slider
2 participants