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

Update the README for the switch package #999

Closed
teoxoy opened this issue Jul 21, 2017 · 8 comments
Closed

Update the README for the switch package #999

teoxoy opened this issue Jul 21, 2017 · 8 comments

Comments

@teoxoy
Copy link

teoxoy commented Jul 21, 2017

The Modifier mdc-switch--disabled specified at the end of the README doesn't do anything. I looked trough the sass file and there is no mention of any class called mdc-switch--disabled.

@teoxoy
Copy link
Author

teoxoy commented Jul 21, 2017

The class mdc-switch-label on the label doesn't do anything either

@amsheehan
Copy link
Contributor

You are correct. Looks like an incomplete implementation to me. The label definitely should do something when a switch is disabled. I checked with design and they recommend using Body1 as the label font, and setting the label on disabled selection components to .38 opacity.

The typography spec

I'm wondering if you'd like to submit a pull request to fix this oversight. Changes would be:

  • Add @include mdc-typography(body1) and opacity: .38 to any mdc-switch-label that is a descendent of mdc-switch--disabled
  • Add the same to mdc-checkbox
  • Add the same to mdc-radio

If you have questions about specific implementations for the other components feel free to ask. If you don't have the cycles let us know. Thanks!

@teoxoy
Copy link
Author

teoxoy commented Jul 26, 2017

@amsheehan Ok so, regarding the checkbox and the radio, they don't have classes for labels (something like mdc-checkbox-label) and I was wondering if it would be better to remove the mdc-switch-label class for simplicity and consistency rather than adding the 2 extra classes for the labels.
And one more thing: the label doesn't have any margin/padding, is it up to the devs using the library to set that or should a default styling exist ?

@dessant
Copy link

dessant commented Aug 3, 2017

The label could be handled by form-field, just like for checkbox and radio.

https://material.io/components/web/catalog/input-controls/form-fields/

@teoxoy
Copy link
Author

teoxoy commented Aug 3, 2017

@dessant the sass file for the form-field package doesn't contain any label styling besides rtl/dark-theme,
the issue is that a label styling is mentioned in the README file of the switch package and it's not implemented. And then it was also suggested to style the labels of the checkbox and radio packages (maybe other input packages too)

@lynnmercier
Copy link
Contributor

@teoxoy I kinda like the idea of having the same class for labels on checkbox, radio, and switch...but we need to check with designers to see if these three components are similar enough. I don't want to couple code together and end up overriding all the time. But my guess is that since all three are Selection Controls, they might be similar enough.

@lynnmercier
Copy link
Contributor

lynnmercier commented Dec 19, 2017

I'm going to split this into two issues

  1. update documentation about disabled switches, because mdc-switch--disabled is not correct
  2. add a demo of mdc-form-field working in concert with mdc-switch (just like we do for mdc-checkbox demo and mdc-radio)
    https://github.com/material-components/material-components-web/blob/master/demos/checkbox.html#L91
    https://github.com/material-components/material-components-web/blob/master/demos/radio.html#L75

We'd like to keep styling of labels in sync between checkbox, radio, and switch with mdc-form-field

@lynnmercier
Copy link
Contributor

And now I'm going to close this issue...see #1805 and #1806 if you want to continue the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants