fix(info-context): InfoBox/InfoWindow children receive React context (addresses #258) #361
Conversation
|
This works fine for me but I noticed am getting an error when I click the As far as I can tell this error isn't breaking anything, but I'm looking into the cause currently. If anyone else is/is not able to replicate the error I'd be interested to hear. ETA: I've figured out the issue and I'm working on patching it. |
|
Patched, errors gone. 😃 |
There was a problem hiding this comment.
wow, thanks for the fix @benwiley4000 ! This could definitely solve #258 .
Looks good in general, I just left two comments for some coding style changes. Also, could you try to split your code changes into two commits:
fix(InfoWindow): allow children to receive React's context
fix(addons/InfoBox): allow children to receive React's context
The first commit should contain changes of src/lib/InfoWindow.js and src/lib/enhanceElement.js. Reason for the changes is for the automatically changelog generation.
You can use git reset HEAD~ to amend your commits and do a force push afterwards, and I'll merge your branch locally to master (I'm hoping it'll mark this PR as merged).
Thanks!
| @@ -72,7 +72,7 @@ const publicMethodMap = { | |||
|
|
|||
| const controlledPropUpdaterMap = { | |||
| children(infoWindow, children) { | |||
| render(Children.only(children), infoWindow.getContent()); | |||
| renderSubtreeIntoContainer(this, Children.only(children), infoWindow.getContent()); | |||
There was a problem hiding this comment.
I'd like to keep it just unstable_renderSubtreeIntoContainer to indicate we're using unstable APIs.
There was a problem hiding this comment.
Also change it to:
children(infoWindow, children, component) {
unstable_renderSubtreeIntoContainer(component, Children.only(children), infoWindow.getContent());
}| @@ -135,7 +135,7 @@ export default _.flowRight( | |||
|
|
|||
| componentDidMount() { | |||
| const infoWindow = getInstanceFromComponent(this); | |||
| controlledPropUpdaterMap.children(infoWindow, this.props.children); | |||
| controlledPropUpdaterMap.children.call(this, infoWindow, this.props.children); | |||
There was a problem hiding this comment.
controlledPropUpdaterMap.children(infoWindow, this.props.children, this);| @@ -87,7 +87,7 @@ const enhanceWithPropTypes = _.curry(( | |||
| _.forEach(controlledPropUpdaterMap, (fn, key) => { | |||
| const nextValue = this.props[key]; | |||
| if (nextValue !== prevProps[key]) { | |||
| fn(getInstanceFromComponent(this), nextValue); | |||
| fn.call(this, getInstanceFromComponent(this), nextValue); | |||
There was a problem hiding this comment.
I'm seeing this fn.call as a bad sign, and I would say, put it in the third argument:
fn(getInstanceFromComponent(this), nextValue, this);
|
@tomchentw thanks for the comments. You're actually putting me much closer to my original code, so go figure. I hesitated to make Also, no need to merge my branch locally. If I do a force push, I believe it should be reflected here and you can merge using GitHub. Don't quote me on that though - we'll find out in a few minutes. |
|
@benwiley4000 the reason to merge locally is:
|
benwiley4000
force-pushed
the
info-child-context
branch
from
7be5a95 to
5748a96
Compare
|
For what it's worth this post claims that should work. Anyway, I've updated my fork. |
There was a problem hiding this comment.
@benwiley4000 this is exactly what I'm looking for.
The changes look great!
* Closes #258 * PR #361 * Ref commit: 5748a96 * Thanks to @benwiley4000
|
Released v6.0.1 in the npm |
Now in
InfoWindoworInfoBoxchildren you can use components like React Router'sLink, which relies on React context for route matching information. Addresses #258.ETA: For this to work all prop updater functions from a component's
controlledPropUpdaterMapare called fromenhanceElementwith a suppliedthis(viaFunction.prototype.call). See here. This wasn't necessary before since none of the updaters requiredthis, but we needthisin order to get React context.