Skip to content

More code review suggestions #251

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.

Sign up for GitHub

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

Open
yeo-yeo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

More code review suggestions #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions app/item/[id]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ export default async function listing({
.from('users')
.select('favourite_items')
.eq('id', userID)
const favouriteItems: number[] | null =
favourites && favourites[0].favourite_items
const favouriteItems = favourites && favourites[0].favourite_items
Copy link
Copy Markdown
Author

@yeo-yeo yeo-yeo Dec 7, 2023

Choose a reason for hiding this comment

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

I haven't systematically been through but I think there a few places like this where you shouldn't need to declare the types so you can just get rid of them


return (
<PageContainer>
Expand Down
3 changes: 1 addition & 2 deletions app/products/[category]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ export default async function Page({
.select('favourite_items')
.eq('id', userID || '')

const favouriteItems: number[] | null =
favourites && favourites[0].favourite_items
const favouriteItems = favourites && favourites[0].favourite_items

return (
<PageContainer justify="justify-start">
Expand Down
2 changes: 1 addition & 1 deletion app/signup/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export default async function Login({
first_name: firstName,
last_name: lastName,
created_at: new Date(),
email: email
email: email,
})
.select()

Expand Down
15 changes: 11 additions & 4 deletions components/buttons/WideFoundationButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,17 @@ export default function WideFoundationButton({
className?: string
}) {
return (
<Button
className={`bg-foundation text-white rounded-full ${className || ''}`}
<Link
Copy link
Copy Markdown
Author

@yeo-yeo yeo-yeo Dec 7, 2023

Choose a reason for hiding this comment

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

Buttons like Message Seller only work if you click on the button text at the moment! I thought there was something wrong with their click handlers/links but then I realised the issue is only the text was being turned into a link

(I might not have got the class/css stuff right here)

href={pageUrl}
className={`bg-foundation text-white text-center rounded-full ${
className || ''
}`}
>
<Link href={pageUrl}>{buttonTitle}</Link>
</Button>
<Button
className={`bg-foundation text-white rounded-full ${className || ''}`}
>
{buttonTitle}
</Button>
</Link>
)
}
15 changes: 5 additions & 10 deletions components/user-info/UserFavourites.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,19 @@ import getItemDetails from '@/utils/fetchItemDetails'
import ItemCard from '@/components/cards/ItemCard'
import fetchSellerName from '@/utils/fetchSellerName'
import getUser from '@/utils/getUser'
import { Item } from '@/utils/types'

export default async function Purchase() {
const { supabase, userID } = await getUser()

let itemDetails: Item[] = []
if (userID) {
itemDetails = (await getItemDetails(
supabase,
'favourite_items',
userID,
)) as Item[]
itemDetails = itemDetails.filter(item => item !== undefined)
if (!userID) {
return null
Copy link
Copy Markdown
Author

@yeo-yeo yeo-yeo Dec 7, 2023

Choose a reason for hiding this comment

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

maybe this isn't the right thing to return, I was being lazy

I think it should be impossible for a logged out user to reach this component so this could actually be a legit time to use userID! on line 13!

}

const itemDetails = await getItemDetails(supabase, 'favourite_items', userID)

const seller_name = await fetchSellerName(
supabase,
itemDetails[0].seller_id || '',
itemDetails?.[0]?.seller_id || '',
Copy link
Copy Markdown
Author

@yeo-yeo yeo-yeo Dec 7, 2023

Choose a reason for hiding this comment

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

With the changes here and in fetchItemDetails I think a bug should be fixed where at the moment if you try to view your favourite items but you don't have any, the whole page crashes

)

return (
Expand Down
35 changes: 20 additions & 15 deletions utils/fetchItemDetails.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createClient } from '@/utils/supabase/server'
import { Item } from '@/utils/types'

export default async function getItemDetails(
supabase: ReturnType<typeof createClient>,
Expand All @@ -10,20 +11,24 @@ export default async function getItemDetails(
.select(column_name as '*')
.eq('id', user_id)

const itemIds: number[] = data?.[0][`${column_name}`] ?? []

if (itemIds) {
const itemDetails = await Promise.all(
itemIds &&
itemIds.map(async item_id => {
const { data } = await supabase
.from('items')
.select('*')
.eq('item_id', item_id)
return data && data[0]
}),
)
return itemDetails
if (!data || data.length === 0) {
return []
}
return []

const itemIds = data[0][`${column_name}`] ?? []

const fetchedItems = await Promise.all(
itemIds.map(async item_id => {
const { data } = await supabase
.from('items')
.select('*')
.eq('item_id', item_id)
return data?.[0]
}),
)

const items = fetchedItems.filter(
(item: Item | undefined): item is Item => item !== undefined,
Copy link
Copy Markdown
Author

@yeo-yeo yeo-yeo Dec 7, 2023

Choose a reason for hiding this comment

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

this here is some pure typescript magic called a 'type predicate'. the item is Item bit means 'what I'm going to return from this little fn is of type Item', and this does away with the annoying (Item | undefined)[] situation you were in before. Just using the filter without this type predicate magic annoyingly doesn't convince Typescript that you've got rid of the undefineds (even though you have)

)
return items
}