feat: allow toggler to be disabled #601
Conversation
acolombier
force-pushed
the
feat/allow-toggler-to-be-disabled
branch
from
d31dd6a
to
c0946f2
Compare
acolombier
force-pushed
the
feat/allow-toggler-to-be-disabled
branch
from
c0946f2
to
856cc5b
Compare
| @@ -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>, | |||
There was a problem hiding this comment.
Since this is optional, it should be moved to a method
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think we should wait for iced to be rebased before making new changes to iced
There was a problem hiding this comment.
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 :)
Depends on #pop-os/iced#169