Skip to content
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

v1 #42

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

v1 #42

wants to merge 4 commits into from

Conversation

marissa-anj
Copy link
Contributor

No description provided.

@marissa-anj
Copy link
Contributor Author

ok this just needs links
rn i am hardcoding a constant url, i dont actually know where group id comes from
id also like to redirect the user to the home page after the save, but im tired so ill test and revise tmrw
Screen Shot 2023-11-23 at 9 46 23 PM
Screen Shot 2023-11-23 at 11 50 28 PM
Screen Shot 2023-11-23 at 9 47 25 PM

@marissa-anj
Copy link
Contributor Author

hover over marker now shows Screen Shot 2023-11-27 at 2 35 27 AM

recommendations data adjusted to receive this time format from the backend
Screen Shot 2023-11-27 at 2 27 49 AM

the group_id and url is dynamically passed to get api call

ISSUES:
-user_id is hardcoded, not sure where this is stored
-the post data looks correct, although I am getting a 405 post method not allowed error
(this might be due to my corsconfig)
Screen Shot 2023-11-27 at 2 27 26 AM

test data in recs was inserting this into sql
INSERT INTO groups_recommendations (activity_id, place_name, place_url, times, group_id_id) VALUES ('Restaurant', 'Mcdonalds', 'https://www.google.com/maps/place/McDonald''s/@43.4653179,-80.5636315,13z/data=!3m1!5s0x882bf3f248c504eb:0xeab7ed8f6958c881!4m10!1m2!2m1!1sgoogle+map+mcdonalds!3m6!1s0x882bf3f2346da8a7:0x305e7d4baf34ec1!8m2!3d43.4817485!4d-80.5255702!15sChRnb29nbGUgbWFwIG1jZG9uYWxkcyIDiAEBWgsiCW1jZG9uYWxkc5IBFGZhc3RfZm9vZF9yZXN0YXVyYW504AEA!16s%2Fg%2F1td013lr?entry=ttu', '["2023-11-20 13:00:00", "2024-01-03 09:00:00", "2023-11-21 04:00:00"]', '5633940446'), ('Restaurant', 'Bao Sandwich Bar', 'https://www.google.com/maps/place/Bao+Sandwich+Bar/@43.4725227,-80.5515083,14.96z/data=!4m9!1m2!2m1!1sgoogle+map+mcdonalds!3m5!1s0x882bf3f652a2a84b:0x5948e3c22a39734c!8m2!3d43.4764771!4d-80.5300292!16s%2Fg%2F11bxd7hmwf?entry=ttu', '["2024-01-03 09:00:00", "2023-11-21 04:00:00"]', '5633940446'), ('Movie Theater', 'Princess Cinema', 'https://www.google.com/maps/place/Princess+Cinemas/@43.4655732,-80.5248735,16.7z/data=!3m1!5s0x882bf40d264bc213:0x58bf7744feab91e4!4m6!3m5!1s0x882bf41278b8fbe3:0x69669374e1015e19!8m2!3d43.4668469!4d-80.5233381!16s%2Fg%2F1w6_pg3g?entry=ttu', '["2023-11-20 13:00:00", "2024-01-03 09:00:00"]', '5633940446');

@marissa-anj
Copy link
Contributor Author

so for the user id i set the local storage test email manually because my google Oauth does not work and i cant test completely in a user setting, so please verify if this works. Thank you!

@@ -16,6 +17,8 @@ function App() {
<Router>
<Routes>
<Route path='/' element={<Home/>} />
<Route path='/activities' element={<Activities/>} />
Copy link
Contributor

@ctlchan ctlchan Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be needed since everything is in the preferences route.

@@ -16,6 +17,8 @@ function App() {
<Router>
<Routes>
<Route path='/' element={<Home/>} />
<Route path='/activities' element={<Activities/>} />
<Route path='/recommendation' element={<Recommendation/>}/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to make it /recommendation:group_id like the others so we can pass through useParams().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally let's make it plural recommendations to be consistent with the other pages. Together /recommendations/:group_id

@@ -59,7 +59,7 @@ function Preferences() {
// TODO: make post request


navigate('/');
navigate('/recommendation');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this can be changed to /recommendation/${group_id}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, let's make it plural recommendations to be consistent with the other pages.

@ctlchan
Copy link
Contributor

ctlchan commented Nov 28, 2023

Couldn't test the actual recommendation returns but I trust that it works fine as you showed. The flow of the routes seems fine barring the missing url parameters.

I noticed the extra links on the home page, which I'm guessing are there just from testing? But some link to routes that ultimately shouldn't exist (i.e., any of the individual preference components).

Copy link
Collaborator

@pabloj2001 pabloj2001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on this page! I have a number of comments but they should all be quick fixes. I noticed that during merging with main you might've overridden some of the API files, we'll need to remove those changes by resetting the branch to a previous commit, merging with main again while ensuring the overriding doesn't happen again, and then applying the latest changes again. We can do it together later.

@@ -59,7 +59,7 @@ function Preferences() {
// TODO: make post request


navigate('/');
navigate('/recommendation');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, let's make it plural recommendations to be consistent with the other pages.

import './index.css';

const apiKey ="AIzaSyABQ4Pci5rnkud1UShQlNtU7iGG7waWghU"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apiKey needs to be removed and turned to an environment variable.

Comment on lines 19 to 28
// If API key is available, dynamically load Google Maps API
const script = document.createElement('script');
script.src = `https://maps.googleapis.com/maps/api/js?key=AIzaSyABQ4Pci5rnkud1UShQlNtU7iGG7waWghU&libraries=places`;
script.async = true;
script.defer = true;

script.onload = renderApp; // Render the app once the Google Maps API script has loaded

document.head.appendChild(script);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused as to why we're using 2 different methods to load the Google Maps API throughout the app. Couldn't we have reused the same method we used for location preferences? Let's leave it for now though for the sake of time.

};

// Perform an API request to save the vote
fetch(import.meta.env.VITE_SERVER + `groups/${group_id}/votes`, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's end all the API calls with a slash / for consistency and compatibility.

Comment on lines 174 to 179
rec_id: {
recommendation_id: selectedRecommendation.id,
selected_time: selectedTime,
},
user_id: userdata,
group_id: group_id,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rec_id is just the id of the recommendation, so selectedRecommendation.id, and user_id should be userdata.user_id. We don't need to pass the group_id since it's given in the URL. I had forgotten about the time in the Votes model, thanks for noticing, we will need to make a new field for it in the API model. Overall I think we only need rec_id, selected_time, and user_id.

})
.then((response) => response.json())
.then((json) => {
setRecommendations(json);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When parsing the json recommendations, I'm afraid it doesn't parse recursively, so the times json array doesn't get parsed to an array and remains as a string. For this reason, we'll have to manually parse all the times fields.

json.map((rec) => {
  rec.times = JSON.parse(rec.times);
});

Something like this before setting the recommendations should do it.

@@ -16,6 +17,8 @@ function App() {
<Router>
<Routes>
<Route path='/' element={<Home/>} />
<Route path='/activities' element={<Activities/>} />
<Route path='/recommendation' element={<Recommendation/>}/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally let's make it plural recommendations to be consistent with the other pages. Together /recommendations/:group_id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants