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 disabled for druid controls #1717

Merged
merged 67 commits into from
May 3, 2021

Conversation

xarvic
Copy link
Collaborator

@xarvic xarvic commented Apr 13, 2021

No description provided.

xarvic and others added 30 commits March 20, 2021 13:51
…ledChanged

 - implemented the disabled state in WidgetPod
 - changed call to focus_change from event to post event processing
 - implemented disabled handling in window.rs and core.rs
 - the focus chain was cleared, if the widget was disabled
(i hope)
Update Documentation

Co-authored-by: Colin Rofls <colin@cmyr.net>
Update documentation

Co-authored-by: Colin Rofls <colin@cmyr.net>
Co-authored-by: Colin Rofls <colin@cmyr.net>
xarvic added 2 commits April 13, 2021 16:42
Signed-off-by: xarvic <xarvix@web.de>
Signed-off-by: xarvic <xarvix@web.de>
@xarvic
Copy link
Collaborator Author

xarvic commented Apr 13, 2021

This is weird. Github believes that i made the change from #1702 here because i merged the branches before creating the PR :/ I hope this will be fine.

xarvic and others added 4 commits April 13, 2021 16:55
Signed-off-by: xarvic <xarvix@web.de>
Signed-off-by: xarvic <xarvix@web.de>
@xarvic xarvic linked an issue Apr 14, 2021 that may be closed by this pull request
@cmyr
Copy link
Member

cmyr commented Apr 15, 2021

@xarvic if you like you could just squash locally? Otherwise we can squash afterwards, I'm not too concerned.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

A few notes inline, but basically I think this is good; I just have two real notes about the styling.

  • Anything that displays text, when disabled, should dim that text. This means the textbox, but also labels anywhere they appear, like the checkbox or the the radio button, or the text in the switch.

druid/src/text/input_component.rs Outdated Show resolved Hide resolved
druid/src/theme.rs Show resolved Hide resolved
druid/src/widget/button.rs Show resolved Hide resolved
druid/src/widget/checkbox.rs Show resolved Hide resolved
druid/src/widget/slider.rs Show resolved Hide resolved
druid/src/widget/switch.rs Show resolved Hide resolved
@SecondFlight
Copy link
Collaborator

Will this resolve #143 once it merges?

@xarvic
Copy link
Collaborator Author

xarvic commented Apr 20, 2021

Yes and it also resolves #746

xarvic and others added 4 commits April 20, 2021 20:35
Signed-off-by: xarvic <xarvix@web.de>
Signed-off-by: xarvic <xarvix@web.de>
Signed-off-by: xarvic <xarvix@web.de>
@xarvic
Copy link
Collaborator Author

xarvic commented Apr 20, 2021

I changed the look:
image
image

@xarvic xarvic requested a review from cmyr April 20, 2021 19:52
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

This looks much better, visually. a few little comments and questions inline, but I think we're very close!

@@ -23,7 +23,9 @@ use crate::{Env, FontDescriptor, FontFamily, FontStyle, FontWeight, Insets, Key}
pub const WINDOW_BACKGROUND_COLOR: Key<Color> =
Key::new("org.linebender.druid.theme.window_background_color");

pub const LABEL_COLOR: Key<Color> = Key::new("org.linebender.druid.theme.label_color");
Copy link
Member

Choose a reason for hiding this comment

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

Because this is heavily used elsewhere (including external projects) I would keep it around with a deprecation notice, like:

#[deprecated(since = "0.8.0", note = "renamed to TEXT_COLOR")]
pub const LABEL_COLOR: Key<Color> = TEXT_COLOR;

@@ -96,6 +96,9 @@ pub struct Label<T> {
pub struct RawLabel<T> {
layout: TextLayout<T>,
line_break_mode: LineBreaking,

control_text: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I would be more explicit about this; it's really only for widget implementors, so I would give it a longer, more explicit name. "is_control_component"?

self.layout.set_text(data.to_owned());
}
LifeCycle::DisabledChanged(disabled) if self.control_text => {
dbg!("Control!");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dbg!("Control!");

/// Builder-style method for making this label into a control-label.
///
/// If this label is a control-label it will change its color when disabled.
pub fn control_text(mut self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

ditto, I'd go with something more explicit maybe? like control_component or even dims_when_disabled?

Did you consider having labels always draw themselves dimmed when they're disabled? I could imagine there maybe being some downsides to this but they aren't totally clear to me..

Copy link
Collaborator Author

@xarvic xarvic Apr 22, 2021

Choose a reason for hiding this comment

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

Did you consider having labels always draw themselves dimmed when they're disabled? I could imagine there maybe being some downsides to this but they aren't totally clear to me..

I don't have a strong opinion how to do this, if you want i change it. But if we do this we still should have the option to disable the color change. We probably don't need this.

Signed-off-by: xarvic <xarvix@web.de>
@xarvic
Copy link
Collaborator Author

xarvic commented Apr 22, 2021

Here is the next version:
image

Signed-off-by: xarvic <xarvix@web.de>
@cmyr
Copy link
Member

cmyr commented Apr 24, 2021

Sorry to let this sit, will try to give it one more thorough look-through tomorrow. :)

@cmyr cmyr mentioned this pull request May 3, 2021
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I think this looks great. If there are any remaining issues we can sort them out afterwards.

Thanks again for taking this on!

@xarvic
Copy link
Collaborator Author

xarvic commented May 3, 2021

No problem, thanks for the review :)

@xarvic xarvic merged commit 99a8d66 into linebender:master May 3, 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.

Add 'disabled' state to widgets.
3 participants