Fix `Form.Check.Label` spacing #5983

TyMick · 2021-08-19T15:40:52Z

Fixes #5938. 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.

This PR 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.

Here's a side-by-side screenshot of the current bug as shown in the documentation site, compared to the fixed version when I run the docs locally after this fix:

EDIT: Only the changes from the first two commits are required to fix this spacing. The third commit adds a related feature that I maybe should've saved for a separate PR. 🙈

TyMick · 2021-08-19T15:46:25Z

Another thing I just thought of is that Form.Check currently doesn't allow users to use a label prop and a custom Form.Check.Input at the same time...

react-bootstrap/src/FormCheck.tsx

Lines 181 to 186 in 3ff951b

    
           {children || ( 
        
             <> 
        
               {input} 
        
               {hasLabel && ( 
        
                 <FormCheckLabel title={title}>{label}</FormCheckLabel> 
        
               )}

...since the presence of any children will prevent a prop-generated label from rendering. Is that something we want to support?

kyletsang · 2021-08-20T16:08:05Z

Wonder if we should allow bools for label here to control the .form-check class in the custom rendering scenario.

@jquense this might be a cleaner approach?

TyMick · 2021-08-20T16:23:13Z

Do you mean requiring the user to pass label={true} to Form.Check when using a custom Form.Check.Label? Just checking if I'm interpreting correctly. 👍🏼

TyMick · 2021-08-20T16:26:05Z

Also I just thought of one way to allow users to use a label prop and a custom Form.Check.Input at the same time (or a custom Form.Check.Label without having to include a custom Form.Check.Input): 5822d94. I can revert if it's too much, though.

kyletsang · 2021-08-21T17:11:12Z

Do you mean requiring the user to pass label={true} to Form.Check when using a custom Form.Check.Label? Just checking if I'm interpreting correctly. 👍🏼

Yeah, so when using custom rendering, setting label={true} would toggle the class

<FormCheck label={true}>
  <FormCheckLabel>Custom</FormCheckLabel>
  ...
</FormCheck>

TyMick · 2021-08-23T14:54:21Z

Right on. As a user, I'd prefer FormCheck to be able to detect a FormCheckLabel child element, but if there's a consensus that that feature isn't worth the overhead of adding a typeName property and a getChildOfType (or includesType) function, I can get behind that.

kyletsang · 2021-08-23T16:07:11Z

@jquense your thoughts on this?

TyMick · 2021-08-23T16:15:30Z

And FWIW, only the changes from the first two commits are required to fix the Form.Check.Label spacing. The third commit adds a related feature that I maybe should've saved for a separate PR. 🙈

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)

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.

TyMick · 2021-10-22T15:31:29Z

I see a stable v2.0.0 was released with Form.Check.Label spacing still broken. 😔 Here's the broken example on the current docs site.

@kyletsang, @jquense, is there anything I can do to help get this fix out the door? I'm willing to refactor to make label={true} add a "form-check" class name to the wrapper if that's what it takes. I just think that Form.Check being able to detect the presence of Form.Check.Label child would be a better developer experience. But this bug is what's keeping my projects on React Bootstrap v1 for the time being.

I've rebased this PR onto the latest master to fix the conflict with #6015.

kyletsang · 2021-10-22T21:44:47Z

@jquense, could we check something like child.type here?

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.

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

TyMick · 2021-10-23T16:49:35Z

@kyletsang You know what, I just learned to my surprise that the type property of a React element has reference equality with a matching imported component variable (JSXElementConstructor object, 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 is indeed all we need to check in getChildOfType; the typeName property I added earlier is not at all necessary.

I've pushed a commit greatly simplifying getChildOfType and removing the typeName property, and I've tested it manually with a local development server of the docs site, with a local production server of the docs site (using the scripts in 5343b0a), and with a personal project, and it still fixes the Form.Check.Label spacing issues! And with a much cleaner solution now.

kyletsang · 2021-10-24T06:01:21Z

src/FormCheck.tsx

-                )}
-              </>
-            )}
+            {inputElement}


This would force the elements to render in a specific order even if the user specified otherwise. For ex:

<Form.Check type={type} id={`check-api-${type}`}> <Form.Control.Feedback type="valid">You did it!</Form.Control.Feedback> <Form.Check.Label>{`Custom api ${type}`}</Form.Check.Label> <Form.Check.Input type={type} isValid /> </Form.Check>

would render

<div class="form-check"> <input type="radio" id="check-api-radio" class="form-check-input is-valid"> <label for="check-api-radio" class="form-check-label">Custom api radio</label> <div class="valid-feedback">You did it!</div> </div>

kyletsang · 2021-10-24T06:06:06Z

You could probably simplify this further to just check the for the label node if children is specified.

Maybe like this?

 const hasLabel =
        (label != null && label !== false && !children) ||
        (children !== undefined &&
          React.Children.toArray(children).find(
            (child) =>
              React.isValidElement(child) && child.type === FormCheckLabel,
          ) !== undefined);

jquense · 2021-10-26T12:24:18Z

Checking type on an element is a bit fraught. It doesn't end up working as nicely as you'd expect. We actually used to do this in a number of components. The problem is that lots of things wrap components.

hot reloading makes this check not work (or used to)
If someone makes their own wrapper around FormCheckLabel it misses
if children contains a wrapping element, or fragment, toArray won't find it.

As a "best effort" its ok but it's not consistent

TyMick · 2021-11-06T01:39:29Z

Thanks for the great feedback, @kyletsang and @jquense! How do things look after this latest commit? I've stopped trying to force the custom render components into a certain order, and now the only thing this PR adds to FormCheck is it checks to see if a direct child is a FormCheckLabel and takes that into account when applying the correct Bootstrap class. Then if someone decides to wrap FormCheckLabel, the official solution can be passing label={true} to FormCheck? I think checking for FormCheckLabel will help people out in the majority of cases, though.

kyletsang

I think this should be ok since it covers the most general case?

Your call @jquense 😄

src/FormCheck.tsx

kyletsang · 2021-11-22T21:49:33Z

@TyMick can you remove the changes in both the package.json files? We don't need those.

I'll merge after

jquense · 2021-11-29T17:28:28Z

sorry i've been so absent. seems ok with me, certainly don't block on me

No idea how that got into 0754b49.

This reverts commit 5343b0a.

TyMick · 2021-11-29T20:08:59Z

@kyletsang Sounds good! I found them useful for testing the production build, but they certainly aren't necessary. 👍🏼

kyletsang · 2021-11-29T20:47:05Z

Thanks! And sorry for the delays 😅

TyMick · 2021-11-29T21:38:17Z

No worries; plenty of those delays were on my end anyway. 😊

TyMick added 3 commits October 22, 2021 08:15

Add error handling to includesType

4085009

TyMick force-pushed the fix-form-check-label-spacing branch from 5822d94 to 3b7a225 Compare October 22, 2021 15:16

TyMick mentioned this pull request Oct 22, 2021

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

Closed

TyMick added 2 commits October 23, 2021 09:38

Add scripts for testing production build of docs

5343b0a

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

TyMick force-pushed the fix-form-check-label-spacing branch from ce25d71 to 5343b0a Compare October 23, 2021 16:39

kyletsang reviewed Oct 24, 2021

View reviewed changes

Just check immediate children for FormCheckLabel

0754b49

kyletsang reviewed Nov 8, 2021

View reviewed changes

src/FormCheck.tsx Outdated Show resolved Hide resolved

Use correct variable

f0d8e9f

TyMick added 2 commits November 29, 2021 12:05

Revert empty .husky/pre-commit change

101e286

No idea how that got into 0754b49.

Revert "Add scripts for testing production build of docs"

1d85fb3

This reverts commit 5343b0a.

kyletsang approved these changes Nov 29, 2021

View reviewed changes

kyletsang merged commit 250655c into react-bootstrap:master Nov 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `Form.Check.Label` spacing #5983

Fix `Form.Check.Label` spacing #5983

TyMick commented Aug 19, 2021 •

edited

TyMick commented Aug 19, 2021

kyletsang commented Aug 20, 2021

TyMick commented Aug 20, 2021

TyMick commented Aug 20, 2021

kyletsang commented Aug 21, 2021

TyMick commented Aug 23, 2021

kyletsang commented Aug 23, 2021

TyMick commented Aug 23, 2021

TyMick commented Oct 22, 2021

kyletsang commented Oct 22, 2021

TyMick commented Oct 23, 2021

kyletsang Oct 24, 2021

kyletsang commented Oct 24, 2021

jquense commented Oct 26, 2021

TyMick commented Nov 6, 2021

kyletsang left a comment

kyletsang commented Nov 22, 2021

jquense commented Nov 29, 2021

TyMick commented Nov 29, 2021

kyletsang commented Nov 29, 2021

TyMick commented Nov 29, 2021

Fix Form.Check.Label spacing #5983

Fix Form.Check.Label spacing #5983

Conversation

TyMick commented Aug 19, 2021 • edited

TyMick commented Aug 19, 2021

kyletsang commented Aug 20, 2021

TyMick commented Aug 20, 2021

TyMick commented Aug 20, 2021

kyletsang commented Aug 21, 2021

TyMick commented Aug 23, 2021

kyletsang commented Aug 23, 2021

TyMick commented Aug 23, 2021

TyMick commented Oct 22, 2021

kyletsang commented Oct 22, 2021

TyMick commented Oct 23, 2021

kyletsang Oct 24, 2021

Choose a reason for hiding this comment

kyletsang commented Oct 24, 2021

jquense commented Oct 26, 2021

TyMick commented Nov 6, 2021

kyletsang left a comment

Choose a reason for hiding this comment

kyletsang commented Nov 22, 2021

jquense commented Nov 29, 2021

TyMick commented Nov 29, 2021

kyletsang commented Nov 29, 2021

TyMick commented Nov 29, 2021

Fix `Form.Check.Label` spacing #5983

Fix `Form.Check.Label` spacing #5983

TyMick commented Aug 19, 2021 •

edited