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

Add radio form elements, make checkbox ids required #65

Merged
merged 2 commits into from
Oct 27, 2017

Conversation

snide
Copy link
Contributor

@snide snide commented Oct 27, 2017

Closes #61
Closes #46

Adds radio inputs to our Form set. They are modeled almost entirely the same as EuiCheckboxGroup. I thought about making them a single mixed component, but realized we'd probably want to do different things with them eventually. The main difference with radios is they only accept a single selectedId instead of a map of ids. While I was in there I make the ids required, per the issue listed above.

While these are functional, I set up a separate issue to track the styling of checkboxes / radios / switches #64. There's enough flexibility in the CSS we can do that at a later time without changing any of the core logic.

@snide snide requested a review from cjcenizal October 27, 2017 02:39
@@ -79,6 +79,10 @@ a {
}
}

input {
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 noticed some weirdness with inputs (I couldn't figure out before why some of our inputs were 2 pixels off). This reset makes sure that our classes are purely additive.

@@ -1,7 +1,6 @@
.euiForm__error {
@include euiFontSizeS;
list-style: disc;
margin-left: $euiSizeXL;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated to this PR, I noticed that our form validation errors had extra margins they didn't need since the addition of EuiText to our EuiCallout component (which automatically apply them to a ul block.

Copy link
Contributor Author

@snide snide left a comment

Choose a reason for hiding this comment

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

Some commentary on the code below.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

One minor suggestion, but overall this LGTM!

background-color: $euiColorLightestShade;

}
&:disabled ~ .euiRadio__label {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about combining this selector with the one above, to make it easier to see how the disabled state changes the component as a whole?

&:disabled {
  & ~ .euiRadio__circle {}
  & ~ .euiRadio__label {}
}

@snide
Copy link
Contributor Author

snide commented Oct 27, 2017

Addressed feedback @cjcenizal, merging.

@snide snide merged commit f0ecced into elastic:master Oct 27, 2017
@bevacqua bevacqua deleted the radio branch October 27, 2017 16:11
@bevacqua
Copy link
Contributor

Amazing stuff @snide! 🎉

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.

3 participants