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 support for custom checkboxes and radios #3343

Merged

Conversation

eladlevy
Copy link

Implemented custom checkboxes and radios mode as discussed in 3047.
Includes tests and documentation

image

@eladlevy eladlevy changed the title Add suppoprt for custom checkboxes and radios Add support for custom checkboxes and radios Oct 22, 2018
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

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

Looks great! @taion how do you feel about adding the custom prop vs a different set of components?

src/FormCheck.js Outdated
@@ -93,6 +97,7 @@ class FormCheck extends React.Component {
{...props}
type={type}
ref={inputRef}
custom={custom}
Copy link
Member

Choose a reason for hiding this comment

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

I think may make sense to put custom on the Context object below with controlId, that way when using the individual components you don't need to explicitly pass custom to the input and label

Copy link
Author

Choose a reason for hiding this comment

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

Great feedback! I wasn't aware of the Context option. This helps to reduce code and makes the customize render option a lot easier to use.

</LinkedHeading>
<p>
Each checkbox and radio is wrapped in a <code>{'<div>'}</code> with a
sibling <code>FormCheckInput</code> to create our custom control and a{' '}
Copy link
Member

Choose a reason for hiding this comment

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

I would shorten this all to something like:

Custom checkbox and radio styles are achieved with a resourceful use of the :checked selector and ::after psuedo elements, but are Structurally similar to the default FormCheck. By default the checked and indeterminate icons use embedded svg icons from Open Iconic...

@eladlevy eladlevy force-pushed the custom-checkboxes-and-radios branch 3 times, most recently from af558d3 to c890043 Compare October 23, 2018 14:50
@taion
Copy link
Member

taion commented Oct 23, 2018

It seems like this is slightly easier than adding e.g. <CustomFormCheck.Input>. That said this seems almost like something bsPrefix can handle, though.

@taion taion closed this Oct 23, 2018
@taion taion reopened this Oct 23, 2018
@taion
Copy link
Member

taion commented Oct 23, 2018

gah i clicked the wrong button

i meant if we had support for bsPrefix from FormCheck cascading down into FormCheck.Input, then had some special-ish (?) logic to also add custom-checkbox or whatever if the bsPrefix is custom-control.

is that too magical? probably? (though i like having that sort of bsPrefix semantics)

in which case i think a custom prop is good

@eladlevy
Copy link
Author

eladlevy commented Oct 24, 2018

@taion hmm I can try overriding bsPrefix in the render method. I thought about something like:

  const activeBsPrefix = custom ? 'custom-control' : bsPrefix;
   <div
          style={style}
          className={classNames(
            className,
            activeBsPrefix,
            custom && `custom-${type}`,
            inline && `${activeBsPrefix}-inline`,
          )}
    >

Is there a better way to override bsPrefix ?
It does saves a bit of logic but if a user passes a custom bsPrefix it will override his prop and may cause unexpected behaviour.
WDYT?

@taion
Copy link
Member

taion commented Oct 24, 2018

no, i was sort of speculating that a "neat" API would be to do something like bsPrefix="custom-control". but i think that's a little too cute.

i'm okay with this API

@eladlevy
Copy link
Author

eladlevy commented Nov 7, 2018

So... @jquense Do you think this PR can be merged?

@jquense
Copy link
Member

jquense commented Nov 8, 2018

Can you rebase?

@eladlevy
Copy link
Author

eladlevy commented Nov 8, 2018

Sure. I've rebased latest master on top of my branch so no dirty merge commits will appear.

@jquense jquense merged commit 97a5b2f into react-bootstrap:master Nov 9, 2018
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.

None yet

3 participants