sderacy / gooncard

The Goon Squad's Goon Card Project for CSC 355 ('22)
http://gooncard.hpc.tcnj.edu:3000/
MIT License
6 stars 2 forks source link

Add API endpoint for returning the user's settings. #85

Closed paytonshaltis closed 2 years ago

paytonshaltis commented 2 years ago

Though the user's settings could be accessed in EJS files normally, making data available in the main.js files requires explicit API endpoints to be set up.

In order to implement settings changes as referenced in issues #7, #8, and #62, an endpoint must be set up to return the currently logged-in user's settings object. This should probably be done in the profile.js file. Below is some information on the endpoint itself:

sderacy commented 2 years ago

@paytonshaltis I had a question and I wanna preface it by saying I totally get it if you want to keep things the way they are, but if we're adding an API call to return the settings, would it be better to keep the logic for displaying the saved settings on the .ejs file or move the logic to the .js file?

I meant it when I said that I was fine with what we have now, but since we're allowing the .js files to use the user's settings, it makes sense to revisit that conversation.

I am keeping in mind that we have a relatively short deadline to implement everything so I understand delaying or not wanting to change what we currently have

paytonshaltis commented 2 years ago

@paytonshaltis I had a question and I wanna preface it by saying I totally get it if you want to keep things the way they are, but if we're adding an API call to return the settings, would it be better to keep the logic for displaying the saved settings on the .ejs file or move the logic to the .js file?

This is actually a really good point; it may make sense to move to the main.js file. I'll create an issue and assign it to Milestone 3, and if we really do end up running low on time we can leave it as is, but I think this is a good thing to refactor.

Good call Sterly!

matyis1 commented 2 years ago

i made a pull request for this issue