-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix react-redux 6.0.0 new way of passing context #113
Fix react-redux 6.0.0 new way of passing context #113
Conversation
assessing multiple PRs regarding the same fix, checking with authors of those prs if there's anything they want to review on this one |
const mapState = state => ({ | ||
routesMap: selectLocationState(state).routesMap | ||
}) | ||
const connector: Connector<OwnProps, Props> = connect(mapState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't dispatch
be mapped to props here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, what do you have in mind?
Do you see a case dispatch is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already used and in a Redux context, dispatching action is needed, I guess. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatch
was not present before, is this necessary?
I wonder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this is officially supported, ok. 👍
src/NavLink.js
Outdated
@@ -37,39 +36,31 @@ type OwnProps = { | |||
|
|||
type Props = { | |||
dispatch: Function, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 39 could be removed, I think.
If still relevant @GuillaumeCisco @cdoublev Please merge with master and resolve conflicts, then ill merge it if the community (you guys) say yes |
Ill merge this if we can resolve the conflicts. Otherwise give me like 4 hours and ill do so on my machine. Again I’m really sorry i got PR's mixed up |
Sounds more efficient that I let you both handle that from your own PR/repository. I should probably have closed my PR @ScriptedAlchemy: also my fault. |
Thanks @cdoublev and @ScriptedAlchemy. |
Sure thing I'll update your pr tomorrow and merge. Tomorrow is my last day before family vacation to the mountains. No idea what wireless service is there. So I'll make sure this is merge tomorrow. Merry Christmas or happy holidays!!! |
5966918
to
fb38fc1
Compare
Just got some time to rebase it :) |
🎉 This PR is included in version 2.1.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Checking in here, we all good? |
Yes, thank you both. I just updated it with react-redux: links are working correctly. 👍 |
Thanks guys. I really appreciatate it! |
Fix #101