Clarify that <fieldset> only uses the disabled attribute #16632

Merged
merged 1 commit into from Jun 18, 2015

Projects

None yet

4 participants

@kkirsche
Contributor

Fixes #16630

Not sure the exact way to word it, but let me know if you'd like adjustments and I'll make those and rebase

@mdo mdo added the docs label Jun 12, 2015
@zacechola
Member

A slight reorganization could work here, by breaking the paragraph into two parts: the attribute and the class, while also maintaining your cross link.

Only the disabled attribute is required on checkboxes, radios, and fieldsets to style the elements appropriately. To have the <label> for the checkbox or radio also display a "not-allowed" cursor when the user hovers over the label, add the .disabled class to your .radio, .radio-inline, .checkbox, .checkbox-inline.

@patrickhlauke
Member

agree with @zacechola

@kkirsche
Contributor

Cool I'll make that change when I get home per your recommendation @zacechola and rebase it. Thanks for the feedback so we can ensure this is clear 👍

@mdo
Member
mdo commented Jun 15, 2015

I think all we need here is:

Disabled checkboxes and radios are supported, but to provide a "not-allowed" cursor on hover, you'll need to add the .disabled class to the parent .radio, .radio-inline, .checkbox, or .checkbox-inline.

It feels odd to mention <fieldset>s here at all. That's for the disabled fieldset docs section, not the checkboxes and radios. Sound about right?

@kkirsche
Contributor

I can understand that @mdo. The thought being why mention something is disabled if the section isn't about the component. Does that sound accurate for the reasoning behind nuking that component from the wording there? If so, that makes sense to me.

@kkirsche
Contributor

Pushed your recommendation @mdo — let me know if any other changes or adjustments are needed or desired.

@mdo mdo and 1 other commented on an outdated diff Jun 15, 2015
docs/_includes/components/input-groups.html
@@ -26,6 +26,7 @@ <h2 id="input-groups-basic">Basic example</h2>
<p>Place one add-on or button on either side of an input. You may also place one on both sides of an input.</p>
<p><strong class="text-danger">We do not support multiple add-ons on a single side.</strong></p>
<p><strong class="text-danger">We do not support multiple form-controls in a single input group.</strong></p>
+ <p><strong class="text-danger"><code>input-group-addon</code> must be the last child within an input group.</strong></p>
@mdo
mdo Jun 15, 2015 Member

Why the addition of this?

@kkirsche
kkirsche Jun 15, 2015 Contributor

That's a fantastic question. No clue where this even came from as I haven't opened that file

@kkirsche
Contributor

removed that weird addition @mdo — not sure where it came from. Sorry about that

@mdo
Member
mdo commented Jun 15, 2015

Guessing there was something wrong with the force push? The commit message is now inaccurate :).

@kkirsche
Contributor

My guess is I had a leftover patch-* branch locally that this overlapped with. That's fixed now though. Sorry about that. I can't wait for V4 so I can remove all my patch-* branches and clean up my local repo without affecting open PRs

@mdo
Member
mdo commented Jun 16, 2015

Thanks for all the back and forth! Sorry for nit picking 😁.

@kkirsche
Contributor

Not nit picking at all. Legitimate mistakes on my part that should have been fixed before even sending it out to you guys. Gonna clean up my branches to only my open PR's so hopefully this won't happen again. Thanks for your patience

EDIT: Branches all cleaned up. Now only have patch-15 (this), patch-18 (CSS Source Maps), patch-16 (License FAQ), and affixoffsetTop0

@mdo mdo modified the milestone: v3.3.5, v3.3.6 Jun 16, 2015
@mdo mdo merged commit 0f6fab0 into twbs:master Jun 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mdo mdo referenced this pull request Jun 18, 2015
Closed

v3.3.6 ship list #16644

@kkirsche kkirsche deleted the kkirsche:patch-15 branch Jun 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment