Conversation
✅ Deploy Preview for redi-love ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jacquelinecai
requested review from
abrar-amin,
glopes2023,
laylaliuuu and
nadia-choop
laylaliuuu
left a comment
laylaliuuu
left a comment
There was a problem hiding this comment.
Nice cleanup! There are a few warnings, maybe we can just delete unused variables and imports to keep the code clean
| type MockDoc = { data: () => typeof mockUserData }; | ||
|
|
||
| const mockGet = (isEmpty: boolean, docs: MockDoc[]) => { | ||
| const mockGet = jest.fn().mockResolvedValue({ |
There was a problem hiding this comment.
I think we have a variable shadowing here, "mockGet" is named twice, maybe we can just rename it just be safe
constants/cornell.ts
Outdated
| @@ -1,4 +1,4 @@ | |||
| import { School, Gender } from '../types'; | |||
| import { Gender, School } from '../backend/types'; | |||
There was a problem hiding this comment.
I just the relative path for this, it seems like to be backend/types.ts (yours probably works too)
maybe just change to that just in case?
backend/src/routes/geocode.ts
Outdated
| const url = `https://api.geoapify.com/v1/geocode/autocomplete?text=${encodeURIComponent(q.trim())}&type=city&limit=5&apiKey=${apiKey}`; | ||
| const response = await fetch(url); | ||
| console.log("test") | ||
| console.log('test'); |
There was a problem hiding this comment.
Should we delete the console log
| }: { | ||
| size?: number; | ||
| color?: string; | ||
| }) => ( |
There was a problem hiding this comment.
maybe just move AnimatedSpinner definition outside of the parent component to get rid of this warning?
| <MotionContext.Provider | ||
| value={{ animationEnabled, setAnimationEnabled }} | ||
| > | ||
| <MotionContext.Provider value={{ animationEnabled, setAnimationEnabled }}> |
There was a problem hiding this comment.
Maybe also just get rid of this warning
frontend/app/(auth)/(tabs)/chat.tsx
Outdated
| @@ -74,7 +74,9 @@ export default function ChatScreen() { | |||
There was a problem hiding this comment.
I think those imports and variables are no longer being used (probably when you delete the duplciated code), maybe just get rid of those too.
| @@ -74,7 +74,9 @@ export default function ChatScreen() { | |||
| const currentUser = getCurrentUser(); | |||
| const [blockedUsers, setBlockedUsers] = useState<Set<string>>(new Set()); | |||
frontend/app/(auth)/edit-profile.tsx
Outdated
There was a problem hiding this comment.
same warning here in this file
| `Admin ${req.user?.email} creating manual match:`, | ||
| matchData | ||
| ); | ||
| console.log(`Admin ${req.user?.email} creating manual match`); |
There was a problem hiding this comment.
since it's admin, should we keep the matchData to keep this console log useful, or just delete it?
|
| genders: | ||
| i % 2 === 0 ? ['female', 'non-binary'] : ['male', 'non-binary'], |
There was a problem hiding this comment.
does this mean that we're inherently opening all users to included non-binary people in their prefs?
| console.log( | ||
| ` ⚠️ ${netid}: Still 0 matches even with relaxed criteria` | ||
| ); |
There was a problem hiding this comment.
Wait should there be more handling for this? Rn it just logs and moves on, but if a user receives 0 matches that seems to be crucial because it's a main feature of our app
| testUsers = await createTestUsers(6); | ||
| const prompt = await createTestPrompt(); | ||
| testPromptId = prompt.promptId; | ||
| await setupUsersAndMatches(6); |
There was a problem hiding this comment.
yayy so nice so much refactoring in this file!
nadia-choop
left a comment
nadia-choop
left a comment
There was a problem hiding this comment.
Thank you for doing sm clean up (definitely much needed on our codebase!) Only had a few small comments/questions, and I think other comments were covered by Layla.
3 participants
Summary
This pull request is the first step towards implementing feature cleanup
Test Plan
tested in dev, all functionality remains the same
Notes
I think for
cornell.ts, I can just bypass that for merge right now. Another consideration is to put all these values into our database, but will need to modify logic a little bit to fetch from there.Breaking Changes