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

fix(radio): add height and vertical-align #4978

Merged
merged 1 commit into from
Dec 15, 2016
Merged

Conversation

pndewit
Copy link
Contributor

@pndewit pndewit commented Dec 14, 2016

Add height and vertical-align to radio buttons so that radio buttons in list actions align correctly with the content. The added styles are already present in the checkbox component which does not have this issue.

Fiddle: https://jsfiddle.net/xuvryLyg/1/
This example is from the documentation: https://getmdl.io/components/index.html#lists-section

Merging this request would also mean a change to the above linked documentation page. Adding the following fix mentioned in that documentation would align the field incorrectly again:

.demo-list-radio {
  display: inline;
}

EDIT: So the piece of CSS above should just be removed from the documentation. The display: inline-block (which is on the radio button component by default) should give the correct result.

Let me know what you think or if I missed something.

Add height and vertical-align to radio buttons so that radio buttons in list actions align correctly with the content. The added styles are already present in the checkbox component which does not have this issue.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@pndewit
Copy link
Contributor Author

pndewit commented Dec 14, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@Garbee
Copy link
Collaborator

Garbee commented Dec 14, 2016

I'm not seeing any changes in the jsfiddle from what we currently have. Even by applying the changes requested. Can you please provide an actual reproduction case for the problem you are seeing that this addresses?

I'm also not seeing any alignment problems in Chrome. Is this related to another engine?

@pndewit
Copy link
Contributor Author

pndewit commented Dec 14, 2016

The jsfiddle is showing the issue for radio buttons in lists. It is aligned lower than the corresponding text left of it. The checkbox (top) and the switch (bottom) are aligned correctly.

I am using Chrome too (54.0.2840.98) on macOS Sierra 10.12.1. This is what the fiddle prints when I run it:
screen shot 2016-12-14 at 14 31 11

EDIT: Just updated Chrome to 55.0.2883.95 and it still shows the same output.

@pndewit
Copy link
Contributor Author

pndewit commented Dec 14, 2016

Sorry, just noticed the fiddle link is incorrect...
Updated link: https://jsfiddle.net/xuvryLyg/1/

@Garbee
Copy link
Collaborator

Garbee commented Dec 14, 2016

So this seems to be OS-specific:

selection_002

Displays properly on Chrome V57 (Dev Channel) on Linux.

I'll leave this to @sgomes to take a peek at on the Mac-side to see if he can reproduce it there in the fiddle. If he can and the proposed changes fixes it then it LGTM since applying them on Linux doesn't cause a problem here.

@Garbee Garbee requested a review from sgomes December 14, 2016 14:05
@pndewit
Copy link
Contributor Author

pndewit commented Dec 14, 2016

Thanks for checking it out! 👍

@maximiliankukla
Copy link

It's misaligned on Chrome V55 for Windows as well.

@pndewit
Copy link
Contributor Author

pndewit commented Dec 14, 2016

@maximiliankukla Could you do a quick check if my changes fix the issue for you?

@sgomes
Copy link
Contributor

sgomes commented Dec 15, 2016

I can see the misalignment on the jsfiddle as well, and the fix doesn't seem to introduce any issues.

Thanks, @pndewit! Merging this in.

@sgomes sgomes merged commit bdb5cae into google:mdl-1.x Dec 15, 2016
@pndewit
Copy link
Contributor Author

pndewit commented Dec 15, 2016

No problem, happy to help! Thanks for resolving the PR so quickly!
Note that the documentation will also need an update: https://getmdl.io/components/index.html#lists-section

Merging this request would also mean a change to the above linked documentation page. Adding the following fix mentioned in that documentation would align the field incorrectly again:

.demo-list-radio {
display: inline;
}
EDIT: So the piece of CSS above should just be removed from the documentation. The display: inline-block (which is on the radio button component by default) should give the correct result.

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

Successfully merging this pull request may close these issues.

5 participants