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

Render checkbox on consumer side #14

Closed
leonderijke opened this issue Apr 29, 2016 · 12 comments
Closed

Render checkbox on consumer side #14

leonderijke opened this issue Apr 29, 2016 · 12 comments

Comments

@leonderijke
Copy link

leonderijke commented Apr 29, 2016

Before 1.0 we were able to render the checkboxes ourselves and pass them as children to CheckboxGroup. I successfully used this in conjunction with react-bootstrap, where react-bootstrap would render the checkboxes.

Since 1.0 it is mandatory to use the Checkbox component to render the checkboxes. Are you planning to support rendering without the Checkbox component as well?

@ziad-saab
Copy link
Owner

@leonderijke I can clearly see how this would be a problem for you. Before saying yes or no let me think about it and see if I can find a solution. If you have any solution to suggest please feel free to do it as well.

Meanwhile you should still be able to use the 0.x version of the library even with React 15. Please let me know if that's not the case...

Thanks for your feedback!

@ziad-saab
Copy link
Owner

@leonderijke for the bootstrap usecase, would it be enough to enable a custom className on the <input type="checkbox"> elements? I'd like to see an example of how you are doing this with <1.0.0. There are many possible solutions to bring back the original functionality :)

@leonderijke
Copy link
Author

Here's an example:

/**
 * Example
 * CheckboxGroup is provided by react-checkbox-group v0.3.2 
 * Checkbox is provided by react-bootstrap v0.29.x
 */
<CheckboxGroup name="fruits" value={this.props.activeFruits}>
    {this.props.fruits.map(fruit => (
        <Checkbox key={fruit._id} value={fruit._id}>{fruit.name}</Checkbox>
    )}
</CheckboxGroup>

The Checkbox component renders not only the input element, but also the containing div and accompanying label. The source of Checkbox is here. So, I just enabling a custom className wouldn't be enough.

My current workaround is to mimic react-bootstrap's output by hand (pasting some snippets):

<CheckboxGroup name="groups" ref={ref => this.groups = ref} defaultValue={this.props.active} onChange={this._handleChange}>
    {this._renderGroupRows}
</CheckboxGroup>

/**
 * Renders the checkboxes for the groups
 *
 * @param {Component} Checkbox Checkbox component provided by `react-checkbox-group`
 */
_renderGroupRows(Checkbox) {
    // Note: rendering Bootstrap HTML by hand here, because `react-bootstrap` and `react-checkbox-group` aren't compatible, in that they both want to output the checkbox
    return (
        <div>
            {this.props.groups.map(group => (
                <div className="checkbox" key={group._id}>
                    <label><Checkbox value={group._id} />{group.name}</label>
                </div>
            ))}
        </div>
    );
}

@ziad-saab
Copy link
Owner

@leonderijke there seems to be a few issues on the react bootstap github mentioning the possibility of a CheckboxGroup:

react-bootstrap/react-bootstrap#342
react-bootstrap/react-bootstrap#1749
react-bootstrap/react-bootstrap#1765

Have you looked at the possibility of creating a CheckboxGroup specifically for react-bootstrap? Since there is some interest, it could be a good opensource contribution :)

What do you think?

@leonderijke
Copy link
Author

Before react-checkbox-group 1.0 there was no need to create a CheckboxGroup specifically for react-bootstrap, as React Checkbox Group was interoperable with React Bootstrap.

Besides React Bootstrap's Checkbox, I can imagine people wanting to use a custom Checkbox component for rendering the checkboxes. Is that a use case you want to support (in a future release)?

For example, in react-mdl they render a complete input with label and desired classnames (source).

It would be very nice if React Checkbox Group would remain interoperable with other libraries in the React ecosystem, instead of creating a specific Checkbox Group for these projects.

@ziad-saab
Copy link
Owner

@leonderijke I see what you mean, and I'm not sure what to answer. The reason why I switched up the model is because the previous version was doing way too many DOM manipulations, and they turned out to be shaky in some of my projects.

If version < 1.0 is working well for you, there is nothing wrong with keeping it. If it turns out that a new version of React breaks it for any reason, I can fix it :)

What do you think of this?

@ziad-saab
Copy link
Owner

@leonderijke as I was typing this I also thought of a new possibility, to let the user provide their own checkbox function. I'll be looking into it as an alternative.

@leonderijke
Copy link
Author

Providing our own checkbox function sounds like a good alternative to me. Let me see if I understand this correctly:

The checkbox function takes the name, the list of checkedValues and an onChange callback and returns a component. The component should be a controlled component, because CheckboxGroup maintains the state. The component should call the passed-in onChange callback with its value.

This gets me thinking about another possibility: use a Higher Order Component to pass name, checked and the bound onChange callback as props to the custom checkbox component.

I'll try to create a little proof of concept, to see if this makes sense.

@ziad-saab
Copy link
Owner

I like the HOC idea :) Let's see if we can do anything with it

Ziad Saab

Full-Stack Web Developer
www.ziad.cc

Instructor / Head of Education
www.decodemtl.com

On Fri, Jun 10, 2016 at 7:41 AM, Leon de Rijke [email protected]
wrote:

Providing our own checkbox function sounds like a good alternative to me.
Let me see if I understand this correctly:

The checkbox function takes the name, the list of checkedValues and an
onChange callback and returns a component. The component should be a
controlled component, because CheckboxGroup maintains the state. The
component should call the passed-in onChange callback with its value.

This gets me thinking about another possibility: use a Higher Order
Component to pass name, checked and the bound onChange callback as props
to the custom checkbox component.

I'll try to create a little proof of concept, to see if this makes sense.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#14 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ADwq5ysGxJ8zF_8l5fmLstkKwZaassPHks5qKU16gaJpZM4ISnQt
.

@leonderijke
Copy link
Author

leonderijke commented Jun 10, 2016

Ok, I have an early version working like this:

  • on CheckboxGroup we add a prop componentClass, to specify a custom checkbox component
  • in the checkbox function we apply the name, checkedValues and onChange to the specified component
  • if no componentClass is specified, we just use the current input in the checkbox function

Example code:

const CustomCheckbox = (props) => (
    <input type="checkbox" {...props} className="my-custom-checkbox" />
);

<CheckboxGroup name="fruit" componentClass={CustomCheckbox}>
        {
            Checkbox => (
                <div>
                    <Checkbox value="kiwi"/>
                    <Checkbox value="watermelon"/>
                </div>
            )
        }
</CheckboxGroup>

The CustomCheckbox component would render react-bootstrap's Checkbox, in my case.

A lot of react-bootstrap components have a similar mechanism for rendering custom components, I've taken some inspiration from that.

What do you think? If this sounds good to you, I'll create a PR.

@ziad-saab
Copy link
Owner

@leonderijke thanks a lot for taking the time to work on this with me. At this time, I've decided to make this component match the API of the react-radio-group component to avoid any confusion when using them both on a project.

@bslinger
Copy link

I'm unclear what this final comment means as I don't use react-radio-group, have you decided to leave this functionality out completely? I would find it incredibly useful to be able to pass in a render function to render each individual checkbox.

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

No branches or pull requests

3 participants