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

<Form.Check> and <Form.Check.Label> should add form-check and form-label class automatically. #5938

Closed
Mizan-Rifat opened this issue Jul 19, 2021 · 8 comments · Fixed by #5983

Comments

@Mizan-Rifat
Copy link

<Form.Check> and <Form.Check.Label> should add form-check and form-label class automatically.

https://react-bootstrap-v5.netlify.app/components/forms/#forms-check-api

@giovannipiller
Copy link
Contributor

I was about to open a similar issue.
Looks like <Form.Check> will only print the class form-check if a label is provided:

label && bsPrefix,

Sometimes it makes sense to not provide a label, for example when an aria-label/aria-labelledby is provided in its place.

@kyletsang
Copy link
Member

So the issue here is that in the custom rendering scenario, the container is missing the form-check class?

@giovannipiller
Copy link
Contributor

giovannipiller commented Jul 20, 2021

As a simple example:

<Form.Check />

Will produce:

<div class="">
  <input type="checkbox" class="form-check-input">
</div>

Which doesn't include the form-check class.


On the other hand, simply adding a label:

<Form.Check label="test label" />

Will produce:

<div class="form-check">
  <input type="checkbox" class="form-check-input">
  <label title="" class="form-check-label">test label</label>
</div>

I think that the class form-check should be displayed regardless of the presence of label.

Reproducible also on 2.0.0-beta-4: https://codesandbox.io/s/zen-silence-7uxg1?file=/src/App.js

@giovannipiller
Copy link
Contributor

(Re-reading it, I might have misunderstood the original issue. I can create a separate one if you'd like.)

@kyletsang
Copy link
Member

@giovannipiller checkboxes without labels don't need the wrapping element with .form-check. This is referenced here: https://getbootstrap.com/docs/5.0/forms/checks-radios/#without-labels

@giovannipiller
Copy link
Contributor

@kyletsang thanks!

TyMick added a commit to TyMick/react-bootstrap that referenced this issue Aug 19, 2021
Currently, when using a `Form.Check.Label` component to customize
`Form.Check` rendering, there will be no space between the checkbox and
the label. This is because `Form.Check` is currently relying on the
presence of a `label` prop to apply the `form-check` class name to the
wrapping `<div>`, because checkboxes without labels [don't need the
wrapping element to have the `form-check` class name][1].

This commit adds a utility to check whether an element has a child of a
certain type. It then uses that utility to check if a `Form.Check`
element has a `Form.Check.Label` child and takes that into account when
determining whether the checkbox has a label.

Adding a special property (currently called `typeName`, but that can
certainly change) to components for this utility is necessary because
React minifies the `displayName` property in production.

[1]: react-bootstrap#5938 (comment)
@imaksp
Copy link

imaksp commented Oct 21, 2021

v2 is now stable but in this official example spacing is not correct, It needs form-check class
https://react-bootstrap.netlify.app/components/forms/#forms-check-api

TyMick added a commit to TyMick/react-bootstrap that referenced this issue Oct 22, 2021
Currently, when using a `Form.Check.Label` component to customize
`Form.Check` rendering, there will be no space between the checkbox and
the label. This is because `Form.Check` is currently relying on the
presence of a `label` prop to apply the `form-check` class name to the
wrapping `<div>`, because checkboxes without labels [don't need the
wrapping element to have the `form-check` class name][1].

This commit adds a utility to check whether an element has a child of a
certain type. It then uses that utility to check if a `Form.Check`
element has a `Form.Check.Label` child and takes that into account when
determining whether the checkbox has a label.

Adding a special property (currently called `typeName`, but that can
certainly change) to components for this utility is necessary because
React minifies the `displayName` property in production.

[1]: react-bootstrap#5938 (comment)
@TyMick
Copy link
Contributor

TyMick commented Oct 22, 2021

@aksdevac I'm trying to get this fixed in #5983. 🤞🏼

kyletsang pushed a commit that referenced this issue Nov 29, 2021
* Fix `Form.Check.Label` spacing

Currently, when using a `Form.Check.Label` component to customize
`Form.Check` rendering, there will be no space between the checkbox and
the label. This is because `Form.Check` is currently relying on the
presence of a `label` prop to apply the `form-check` class name to the
wrapping `<div>`, because checkboxes without labels [don't need the
wrapping element to have the `form-check` class name][1].

This commit adds a utility to check whether an element has a child of a
certain type. It then uses that utility to check if a `Form.Check`
element has a `Form.Check.Label` child and takes that into account when
determining whether the checkbox has a label.

Adding a special property (currently called `typeName`, but that can
certainly change) to components for this utility is necessary because
React minifies the `displayName` property in production.

[1]: #5938 (comment)

* Add error handling to `includesType`

* Support mixing auto and custom child components

Currently, `Form.Check` doesn't allow users to use a custom
`Form.Check.Input` with a label generated by the `label` prop (and
requires that a custom `Form.Check.Input` be used if a custom
`Form.Check.Label` is also used) because the presence of _any_ children
in `Form.Check` prevents automatic inputs and labels (and feedback) from
generating. This approach allows mixing and matching by extracting any
custom input, label, and feedback components from `Form.Check`'s
`children` prop, then separately for each, using the custom component if
provided or an automatic component otherwise.

* Simply check `child.type`

The `type` property of React elements have reference equality with
matching imported component variables (`JSXElementConstructor` objects,
to be precise)! For some reason I was not expecting that, but it makes
sense to me now, since these constructor objects are being imported from
a singular source, so there's no reason React would create multiple
instances of them. So `child.type === type` all we need to check in
`getChildOfType`; the `typeName` prop I added earlier is not at all
needed.

* Add scripts for testing production build of docs

Just as a convenience. I can revert this commit if these scripts aren't
desired.

* Just check immediate children for `FormCheckLabel`

* Use correct variable

* Revert empty `.husky/pre-commit` change

No idea how that got into 0754b49.

* Revert "Add scripts for testing production build of docs"

This reverts commit 5343b0a.
Brendan-csel added a commit to solid-libs/solid-bootstrap that referenced this issue Dec 1, 2021
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 a pull request may close this issue.

5 participants