chenglou / react-radio-group

Better radio buttons.
MIT License
445 stars 75 forks source link

context based solution #32

Closed danielberndt closed 8 years ago

danielberndt commented 8 years ago

This should be everything that is required to make a context based solution work.

notes:

All in all I find this solution much easier on the end user's side. Plus it fixes #12!

chenglou commented 8 years ago

Yeah seems legit!

I personally don't use this module anymore, but since you seem to, I've added you to the repo contributors and npm pushers list. Feel free to merge this and publish (and update the README).

Btw, shouldComponentUpdate would simply take context into consideration, right?

danielberndt commented 8 years ago

Cool thanks! :)

Regarding your question on shouldComponentUpdate I'm not sure I get what situation you are pointing to. However here's a use case that's gonna be problematic, I think:

<RadioGroup selectedValue={foo}>
  <Box>
    <Radio value="apple"/> Apple
    <Radio value="orange"/> Orange
  </Box>
</RadioGroup>

if <Box> implements shouldComponentUpdate the default way, i.e. shallowEqual(this.props, nextProps) && shallowEqual(this.state, nextState), changing the selectedValue will not propagate through to the <Radio>s because no props have changed for <Box>.

There's this big issue talking about this, but it's still an ongoing conversation.

I suggest that if we should go down the context-based path, to put a warning into the readme that this library might break if you use any components between <RadioGroup> and <Radio> that implement shouldComponentUpdate. (which seems unlikely, yet it's probably gonna happen)

nkbt commented 8 years ago

Looks good to me (with comments about naming and defaultProps).

danielberndt commented 8 years ago

Changed the comp prop to Component and updated the README.md. I did not change the links within the README pointing to the code yet, since we don't have a permalink for the commits yet.

Please let me know what you think!

chenglou commented 8 years ago

Great. I'll go ahead and merge this then! Thanks a lot. If you want to publish this, feel free to do so. The scripts are still there (although not as modern. They still work I think? @nkbt knows all about this =]). Feel free to close relevant issues too.

danielberndt commented 8 years ago

I just packaged everything up and released v3.0.0, it seems however that the build is broken 😞

The umd build is still assuming one default export whereas now there are two named exports. I'll research to see how to update the configuration.

danielberndt commented 8 years ago

Okay, I more or less copied react-router's publishing method and released a working version v3.0.1. Not sure whether to unpublish v3.0.0 because that version won't help anyone...

nkbt commented 8 years ago

Don't unpublish please, not good anyway... Is it ok now?

nkbt commented 8 years ago

Tip: to avoid problems with publishing always check with npm install ~/path/to/lib somewhere else and check if all is good.

danielberndt commented 8 years ago

Thanks for the tip! Bookmarked it :)

installing the newest version worked fine on my application, but there seemed to be a problem here: #31 let's see what their answer is.

axelson commented 8 years ago

I just wanted to thank you for this new API, much nicer to use! 👍