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.

Sign up for GitHub

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

feat: allow toggler to be disabled #601

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: allow toggler to be disabled #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
856cc5b
feat: allow toggler to be disabled
acolombier Sep 7, 2024
51b7a94
Merge remote-tracking branch 'upstream/master' into feat/allow-toggle…
acolombier Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion iced
Open in desktop
Submodule iced updated 5 files
+6 −1 graphics/src/text/editor.rs
+31 −0 style/src/theme.rs
+5 −0 style/src/toggler.rs
+1 −1 widget/src/helpers.rs
+14 −6 widget/src/toggler.rs
24 changes: 24 additions & 0 deletions src/theme/style/iced.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use crate::theme::{CosmicComponent, Theme, TRANSPARENT_COMPONENT};
use cosmic_theme::composite::over;
use iced::color;
use iced_core::{Background, Border, Color, Shadow, Vector};
use iced_style::application;
use iced_style::button as iced_button;
Expand Down Expand Up @@ -858,6 +859,29 @@ impl toggler::StyleSheet for Theme {
..self.active(style, is_active)
}
}

fn disabled(&self, style: &Self::Style, is_active: bool) -> toggler::Appearance {
let theme = self.cosmic();
const HANDLE_MARGIN: f32 = 2.0;
let background: Color = if is_active {
theme.accent.base.into()
} else {
theme.palette.neutral_5.into()
};
let foreground: Color = theme.palette.neutral_2.into();
toggler::Appearance {
background: Color::from_rgba(background.r, background.g, background.b, 0.5),
background_border: None,
foreground: Color::from_rgba(foreground.r, foreground.g, foreground.b, 0.5),
foreground_border: None,
border_radius: theme.radius_xl().into(),
handle_radius: theme
.radius_xl()
.map(|x| (x - HANDLE_MARGIN).max(0.0))
.into(),
handle_margin: HANDLE_MARGIN,
}
}
}

/*
Expand Down
2 changes: 1 addition & 1 deletion src/widget/settings/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,6 @@ impl<'a, Message: 'static> Item<'a, Message> {
is_checked: bool,
message: impl Fn(bool) -> Message + 'static,
) -> Row<'a, Message> {
self.control(crate::widget::toggler(None, is_checked, message))
self.control(crate::widget::toggler(None, is_checked, Some(message)))
}
}
2 changes: 1 addition & 1 deletion src/widget/toggler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use iced_core::text;
pub fn toggler<'a, Message, Theme: iced_widget::toggler::StyleSheet, Renderer>(
label: impl Into<Option<String>>,
is_checked: bool,
f: impl Fn(bool) -> Message + 'a,
f: Option<impl Fn(bool) -> Message + 'a>,
Copy link
Member

Choose a reason for hiding this comment

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

Since this is optional, it should be moved to a method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense, especially as this will help not making breaking change to the API. Just to confirm, are you happy with having two functions, like:

  • toggler(label, is_checked, closure): Toggler (existing)
  • toggler(label, is_checked): Toggler

And add a method (and maybe a *_maybe variation):

  • Toggler::on_change(impl Fn(bool) -> Message + 'a)

Is that what you had in mind?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to make breaking changes where necessary. There should be a method if we make the message optional. But we may want to wait for the iced rebase before we do this.

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 see so better hold off and go ahead with the current state? Or would you still want to see the Toggler::on_change method in that PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should wait for iced to be rebased before making new changes to iced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Would you suggest blocking this PR and dependencies then? Appreciate there currently seems to be a blocker on iced rebase, so perhaps this change could go in as is, since it included minimal requirement for toggle matching an acceptable UX for async operation.
Alternatively, thinking about the Bluetooth screen, I could put something like the text("...") with the toggler is not available.

Appreciate all options are sub-optimal here, but just keen to get the ball rolling for COSMIC :)

) -> widget::Toggler<'a, Message, Theme, Renderer>
where
Renderer: iced_core::Renderer + text::Renderer,
Expand Down