Basic redux integration for thread component #141
Conversation
web/app/components/thread/thread.jsx
Outdated
| data: { comment, replies = [] }, | ||
| mods = {}, | ||
| } = props; | ||
|
|
||
| let threadComments = null; | ||
| if (!collapsed && !!replies.length) { | ||
| threadComments = replies.map(thread => ( |
There was a problem hiding this comment.
I prefer inline variant. Please put it back to render
| const currentCollapsed = state[action.comment.id]; | ||
|
|
||
| if (action.collapsed !== currentCollapsed) { | ||
| persistCollapsedComments(action.comment, action.collapsed); |
There was a problem hiding this comment.
Reducers must be pure functions. No side effects allowed.
There was a problem hiding this comment.
We can make custom middleware to persist to localStorage. Using localStorage value for initial value is ok.
There was a problem hiding this comment.
Agreed, done. If there will be more cases like this - we could try handling all the side effects with sagas
web/app/components/thread/thread.jsx
Outdated
| mapDispatchToProps | ||
| ); | ||
|
|
||
| export default enhance(Thread); |
There was a problem hiding this comment.
I also prefer inline declarations here as well:
export default connect(
(state, props) => ({
collapsed: getThreadIsCollapsed(state, props.data.comment)
}),
{ setCollapse }
)(Thread);There was a problem hiding this comment.
Fixed. Hope it won't become unreadable as soon as we wrap it up with one more HOC :) Please note that we need this hack with <ConnectedThread cause we're using the recursion here
DmitryTsepelev
force-pushed
the
redux
branch
3 times, most recently
from
4f195ca
to
3f17589
Compare
|
@Guria Thanks for the review, I think I'm done. Please let me know if we need to change something else |
There was a problem hiding this comment.
Ok, it seems it's good first move.
Though it's better to left only one store as soon as possible :)
Also please attach to redux and react devtools.
It would be enough just import 'preact/debug' in each entry point and compose another middleware to redux store.
web/app/store.js
Outdated
| ...threadReducers, | ||
| }); | ||
|
|
||
| const collapsedCommentsMiddleware = ({ getState }) => next => action => { |
There was a problem hiding this comment.
maybe export middleware from thread as well?
DmitryTsepelev
force-pushed
the
redux
branch
3 times, most recently
from
d6ca1fb
to
67a367e
Compare
|
@Guria I'm stuck with the devtools :( I've configured everything according to the docs, aliased all the react-related things, but redux dev tool does not see the store, while react dev tool is "waiting for react" or "waiting for the roots to load", what am I missing here? |
|
Ok, nevermind. We can do it in a separate PR. But we need to resolve new conflicts here. |
|
Done |
|
@Guria Are we good to go? :) I have one more PR to submit |
|
@DmitryTsepelev please don't use yarn. only npm 6.1.0+ |
|
ah ok, got it |
Here is a very basic integration with redux. I've moved all the complex logic from the Thread component to the reducer, which is also responsible for restoring the initial state from the local storage. Reducer is already covered with a couple of specs, would be also useful to have a spec for getters. Related to #95 and #96