Pushing for help with Redux selectors #880

Merged
daovist merged 6 commits from master into master 2017-12-19 21:09:59 +01:00
daovist commented 2017-12-18 18:13:08 +01:00 (Migrated from github.com)

I watched a Redux video and read some of the docs. I've created my action (please ignore the do and confirm actions, I will delete those, sorry) and reducer and they seem to be working but my selector is not. I could use help on this. I think once I manage to get the videoPause value from the store into the VideoPlayer component I will be able to wrap this up.

I watched a Redux video and read some of the docs. I've created my action (please ignore the do and confirm actions, I will delete those, sorry) and reducer and they seem to be working but my selector is not. I could use help on this. I think once I manage to get the videoPause value from the store into the VideoPlayer component I will be able to wrap this up.
liamcardenas commented 2017-12-18 18:52:23 +01:00 (Migrated from github.com)

I'm looking into this

I'm looking into this
liamcardenas (Migrated from github.com) requested changes 2017-12-18 19:13:32 +01:00
liamcardenas (Migrated from github.com) commented 2017-12-18 19:12:13 +01:00

what if you change this to componentWillReceiveProps instead of pauseVideo?

what if you change this to `componentWillReceiveProps` instead of `pauseVideo`?
liamcardenas (Migrated from github.com) commented 2017-12-18 19:13:27 +01:00

then of course you would need to filter out the prop changes that don't lead to a pause, but does that make sense?

then of course you would need to filter out the prop changes that don't lead to a pause, but does that make sense?
daovist (Migrated from github.com) reviewed 2017-12-18 20:01:12 +01:00
daovist (Migrated from github.com) commented 2017-12-18 20:01:11 +01:00

I tried that and the method(?) was never called.

I think the issue is that the selector isn't working, or that the reducer isn't actually changing the state. My focus is on line 63 of components/video.view.jsx where it console logs the value of videoPause. It's always false, even after I've clicked open, which should update valuePause to true through the reducer.

I tried that and the method(?) was never called. I think the issue is that the selector isn't working, or that the reducer isn't actually changing the state. My focus is on line 63 of components/video.view.jsx where it console logs the value of videoPause. It's always false, even after I've clicked open, which should update valuePause to true through the reducer.
kauffj (Migrated from github.com) reviewed 2017-12-18 23:00:25 +01:00
kauffj (Migrated from github.com) commented 2017-12-18 23:00:25 +01:00

@daovist try using the console to help debug what unexpected behavior is happening here. The console will show all action dispatches as well as all state changes (expand the console messages that are printed on action dispatches). This should tell you whether it's an issue with actions or the reducer. If both the action is fired and the Redux state looks correct, you can add a console statement to see if the selector is firing when you think it ought to.

@daovist try using the console to help debug what unexpected behavior is happening here. The console will show all action dispatches as well as all state changes (expand the console messages that are printed on action dispatches). This should tell you whether it's an issue with actions or the reducer. If both the action is fired and the Redux state looks correct, you can add a console statement to see if the selector is firing when you think it ought to.
liamcardenas (Migrated from github.com) requested changes 2017-12-19 00:01:25 +01:00
liamcardenas (Migrated from github.com) commented 2017-12-18 23:59:57 +01:00

this should go in videos/index.js

this should go in videos/index.js
@ -56,6 +56,8 @@ class Video extends React.PureComponent {
changeVolume,
liamcardenas (Migrated from github.com) commented 2017-12-19 00:00:57 +01:00

you want this in here, and then you want to pass it into VideoPlayer

you want this in here, and then you want to pass it into VideoPlayer
liamcardenas (Migrated from github.com) requested changes 2017-12-19 00:23:59 +01:00
liamcardenas (Migrated from github.com) commented 2017-12-19 00:23:53 +01:00

this should be

const _selectState = state => state.video || {};

this should be `const _selectState = state => state.video || {};`
liamcardenas (Migrated from github.com) requested changes 2017-12-19 20:51:06 +01:00
liamcardenas (Migrated from github.com) left a comment

Looks great! just this one question/change

Looks great! just this one question/change
@ -23,0 +23,4 @@
componentWillReceiveProps(nextProps) {
if (nextProps.videoPause) {
this.refs.media.children[0].pause();
this.props.setVideoPause(false);
liamcardenas (Migrated from github.com) commented 2017-12-19 20:47:37 +01:00

is this (this.props.setVideoPause(false);) necessary? can we trigger this, instead, when they push "play" in app?

is this (`this.props.setVideoPause(false);`) necessary? can we trigger this, instead, when they push "play" in app?
liamcardenas (Migrated from github.com) approved these changes 2017-12-19 21:09:32 +01:00
liamcardenas commented 2017-12-19 21:09:52 +01:00 (Migrated from github.com)

Making a follow up issue to this since this is work.

Making a follow up issue to this since this is work.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-desktop#880
No description provided.