wip #40 - Added nav bar #52
Conversation
simon-bourque
requested review from
jacobrs,
MartinSpasov,
ONourry and
samantha-kerr
| @@ -14,12 +14,15 @@ const UserReducer = (state = initialState, action) => { | |||
| let data = [...state.data]; | |||
| if(action.response.statusCode == 200){ | |||
| data.user = action.response.user; | |||
| state.loggedIn = true; | |||
There was a problem hiding this comment.
why not just check if the data.user prop is set? also later on we will be using cookies for this so it might require additional refactoring.
There was a problem hiding this comment.
That's what I wanted todo, but thing is data.user is not declared until the first logging, but im having trouble adding data.user to the initial state. I think we need to fix it up, Is data necessary, can we just have user instead of data.user?
There was a problem hiding this comment.
data isn’t necessary but since redux only keeps one giant store, adding sub-objects is our way of creating sub-stores.
There was a problem hiding this comment.
the sub-store is users, so it would be users.data.user, and also data is an empty array so its weird, were adding user as a member of data and not in the array itself
There was a problem hiding this comment.
Hmm, yeah that is weird, we should probably make it an empty object instead. Why do you suggest users.data.user seems like an even larger complication no?
There was a problem hiding this comment.
Requires redesign in my opinion lol.
There was a problem hiding this comment.
I agree as well, this is what I wanted to bring up with my initial comment lol, but i think redux is the one splitting it by reducer, but if everyone gives me permission id like to remove users.data.user and replace it with just users.user
There was a problem hiding this comment.
that way it becomes trivial to add user : null to the initial state and I can just use a simple null check for the navbar
There was a problem hiding this comment.
I see benefit in refactoring this, just make sure to create an issue for it.
There was a problem hiding this comment.
There was a problem hiding this comment.
The logout redirects but doesn't remove the loggedIn value from the store. Thought that should be noted but isn't really part of this issue. Apart from that, looks good, might need some additional design work from @sam so I wouldn't close the issue yet.
|
@jacobrs Yup it still needs the design, its just a placeholder for now, a log out action is still required as well |
I added the nav bar and it only shows up when the user logs in, We need to do some cleanup in UserReducer because I really needed to add a new boolean to get the nav bar to hide properly. The way the json is set up for our store is very messy and all over the place atm.
I rather just be able to check whether user in our store is null or not as opposed to having an extra boolean but its not in the initial state and its added in a very weird place (data.user but data is an array?)