-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
…r interface for future decoupling from other user endpoints
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.
i like it. so is the plan then to basically make the authenticated user thing part of a context when logged in so it gets stored and stuff so we only need it once?
Yeah basically. I guess it kinda depends on how we expand user settings and what we store in there. We would separately enable editing of user settings via the settings page. |
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.
This is very cool. Here are some initial notes, haven’t gone fully in depth but I’ve looked at everything so far:
- Will want to get rid of the top theme picker
- Thomas emrax’s theme doesn’t load in as light on my local even if it’s set as his default theme
- I think that all users will naturally have no settings because it’s optional and then all the endpoints will not work because they assume it already exists, when they should probably create a default settings if it doesnt exist
I figured we could keep it. did you think it's better removed? cuz if we remove it then there's technically no way to change your current theme without changing your default theme, right? Is this what you'd want?
hmmm, any ideas why?
I was figuring we would fix this by manually generating settings for everyone when we deploy this, which I think is no biggie given the manual script |
ok so I see why it doesn't work on dev signing and im pretttty sure this will work on prod. The one commit I made was just so that it uses the chosen theme instead of the default theme value in the hook looks good to me! |
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.
our auth system is kinda elaborate
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.
nice
Changes
Added user settings to the database model and prisma and seed data.
Okay so basically my plan here was to separate out and have the login endpoint be what first returns the user settings, since this would avoid an extra call to the server and allow us to set up the app with the user's preferred settings as soon as they log in.
This also goes along with my plan to change around the user endpoints to remove sending unnecessary info to the front-end whenever possible. Having a singular unified User interface makes that somewhat difficult, so this take a first step to try and have a separate interface for authenticated user and see how things go.
Screenshots
instant change post-change
preserved on reload, theme is loaded on login
Checklist
It can be helpful to check the
Checks
andFiles changed
tabs.Please review the contributor guide and reach out to your squad if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
package-lock.json
changes (unless dependencies have changed)