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

Minor Scrollable Improvements #1878

Merged
merged 4 commits into from
Jun 27, 2023
Merged

Minor Scrollable Improvements #1878

merged 4 commits into from
Jun 27, 2023

Conversation

AustinMReppert
Copy link
Contributor

This PR provides minor improvements for the Scrollable widget.

  • Align vertical and horizontal properties. Vertical properties are now optional and None by default.
  • Prevent mouse wheel scrolling when vertical/horizontal properties are None.

@tarkah
Copy link
Member

tarkah commented May 27, 2023

Prevent mouse wheel scrolling when vertical/horizontal properties are None.

Good catch on this!

@AustinMReppert
Copy link
Contributor Author

What is the purpose of changing the mouse coordinates like this?

Point::new(-1.0, -1.0)

This will cause strange behaviors when trying scroll with the mouse outside of the window bounds with nested scrollables.

use iced::widget::scrollable::{Properties, Viewport};
use iced::widget::{Container, Scrollable};
use iced::{
    executor, widget, Application, Background, Color, Command, Element, Length, Settings, Size,
    Theme,
};

pub fn main() -> iced::Result {
    HelloWorld::run(Settings::default())
}

#[derive(Debug, Clone, Copy)]
pub enum Message {
    Scrolled(Viewport),
}

struct HelloWorld {}

impl Application for HelloWorld {
    type Executor = executor::Default;
    type Message = Message;
    type Theme = Theme;
    type Flags = ();

    fn new(_flags: Self::Flags) -> (Self, Command<Self::Message>) {
        (HelloWorld {}, Command::none())
    }

    fn title(&self) -> String {
        String::from("Hello World!")
    }

    fn update(&mut self, _message: Message) -> Command<Self::Message> {
        Command::none()
    }

    fn view(&self) -> Element<Message> {
        Scrollable::new(
            Scrollable::new(
                Container::new("Foo")
                    .style(|theme: &iced::Theme| widget::container::Appearance {
                        text_color: None,
                        background: Some(Background::Color(Color::new(1.0, 0.2, 0.3, 1.0))),
                        border_radius: Default::default(),
                        border_width: 0.0,
                        border_color: Default::default(),
                    })
                    .width(Length::Fixed(5000.0))
                    .height(Length::Fixed(5000.0)),
            )
            .on_scroll(Message::Scrolled)
            .width(Length::Fill)
            .height(Length::Fill)
            .vertical_scroll(Properties::default())
            .horizontal_scroll(Properties::default()),
        )
        .into()
    }

    fn theme(&self) -> Theme {
        Theme::default()
    }
}

@bungoboingo
Copy link
Contributor

bungoboingo commented May 29, 2023

Align vertical and horizontal properties. Vertical properties are now optional and None by default.

I also messed with making vertical scroll optional when I was adding horizontal support, but couldn't see the usefulness of a situation where a Scrollable couldn't scroll (e.g. both directions are None). Originally I made an enum that was like:

enum ScrollDirection {
    #[default]
    Vertical,
    Horizontal,
    Both,
}

which might be a better representation of how it's actually being used?

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

@bungoboingo I agree. A scrollable that does not scroll does not make sense. Use another widget instead.

Also, we definitely do not want scrollable to disable scrolling on both axes by default, which is what this PR is currently doing.

@hecrj hecrj added feature New feature or request widget labels Jun 27, 2023
@hecrj hecrj added this to the 0.10.0 milestone Jun 27, 2023
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks 🙇

I have just renamed ScrollbarProperties to simply Direction and simplified some stuff here and there.

We can merge this! 🚢

@hecrj hecrj enabled auto-merge June 27, 2023 21:06
@hecrj hecrj merged commit 9d83718 into iced-rs:master Jun 27, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request widget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants