-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modernize redux: userSlice #242
Modernize redux: userSlice #242
Conversation
Switching to redux toolkit for getting users for the user table.
Turns out even more of the parameters for the addNotification function in the notificationThunks are perfectly optional.
Moving code to where I think it belongs
Moving code to where I think it belongs
Which allows us to get rid of userThunks.ts
Fixes a "can't access lexical declaration 'X' before initialization" error, by resolving a kind of circular dependency.
This pull request has conflicts ☹ |
Adds typing on what we expect the backend to return for users.
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.
As requested, I tested these redux PRs (without looking at the code this time), just to see if anything breaks with these. We talked about any issues I encountered internally, so I won't repeat those here (since they are either unrelated to a specific PR or have been fixed). If anyone is interested, my findings and Arne's comments are documented here.
These have been open for quite a while now and needed to get tested by another person before they are merged. Since everything seems to be working I'll approve this and the other ones.
You'll see this a lot on the other redux PRs, this will be my only comment mentioning the above stuff.
This pull request has conflicts ☹ |
This pull request has conflicts ☹ |
This pull request has conflicts ☹ |
…oolkit-user-reducer
Switching to redux toolkit for getting users for the user table.
Helps with #213, based off of #214.