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

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

Merged
merged 1 commit into from Jun 18, 2015

Conversation

kkirsche
Copy link
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
Copy link

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
Copy link
Member

agree with @zacechola

@kkirsche
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

Why the addition of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@kkirsche
Copy link
Contributor Author

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

@mdo
Copy link
Member

mdo commented Jun 15, 2015

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

@kkirsche
Copy link
Contributor Author

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
Copy link
Member

mdo commented Jun 16, 2015

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

@kkirsche
Copy link
Contributor Author

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 milestones: v3.3.5, v3.3.6 Jun 16, 2015
mdo added a commit that referenced this pull request Jun 18, 2015
Clarify that <fieldset> only uses the disabled attribute
@mdo mdo merged commit 0f6fab0 into twbs:master Jun 18, 2015
@mdo mdo mentioned this pull request Jun 18, 2015
@kkirsche kkirsche deleted the patch-15 branch June 18, 2015 23:24
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.

None yet

4 participants