User profile#62
Conversation
i30101
left a comment
i30101
left a comment
There was a problem hiding this comment.
Overall a good start. One thing I'd generally note is to be careful about what you're pushing - both which files and what's in them. Ideally PRs should be small incremental changes, so you want to minimize unnecessary changes (especially like the whitespace edits in various files). Good job familiarizing yourself with the codebase, though.
| @@ -0,0 +1,66 @@ | |||
| import Image from "next/image"; | |||
| import { Star, Mail, Phone } from "lucide-react"; | |||
There was a problem hiding this comment.
remove unused imports, this is another thing I'd check in my files before committing
| @@ -0,0 +1,48 @@ | |||
| import Link from "next/link"; | |||
There was a problem hiding this comment.
Consider adding bg to this component to distinguish itself from the background. It doesn't look bad right now, but just something I'd think about
jamesdoh0109
left a comment
jamesdoh0109
left a comment
There was a problem hiding this comment.
Good first pr - left some comments
| title="Saved Posts" | ||
| count={savedPosts.count} | ||
| listings={savedPosts.results} | ||
| seeAllHref="/profile" |
There was a problem hiding this comment.
Do we not have a separate page for saved posts?
There was a problem hiding this comment.
Is this component used anywhere?
| function maskPhoneNumber(phone: string): string { | ||
| const digits = phone.replace(/\D/g, ""); | ||
| if (digits.length < 4) return phone; | ||
| const areaCode = digits.slice(digits.length - 10, digits.length - 7); | ||
| const prefix = digits.slice(digits.length - 7, digits.length - 4); | ||
| return `(${areaCode}) ${prefix}-XXXX`; | ||
| } |
There was a problem hiding this comment.
Why should we mask phone numbers for user profile page?
There was a problem hiding this comment.
Agreed, when viewing other peoples' profiles we might want masking but it's not needed here
There was a problem hiding this comment.
Right now, the listings are horizontally scrollable - im wondering if this is necessary if we have "Show all my listings" button. What do you think?
There was a problem hiding this comment.
Would say the same, I think it would be simpler if clicking on "My Listings" or "Saved Posts" sent you to their respective pages instead of what we have now
| <ListingsCard | ||
| key={listing.id} | ||
| listing={listing} | ||
| previewImageUrl={listing.images[0]} | ||
| href={`/${listing.listing_type === "item" ? "items" : "sublets"}/${listing.id}`} |
There was a problem hiding this comment.
I think the card ratio looks kinda weird here? Maybe make it bigger?
| return new Date(listing.expires_at) > new Date(); | ||
| } | ||
|
|
||
| export const MyListingsContent = ({ listings }: Props) => { |
There was a problem hiding this comment.
I think it's fine for now how you are showing all my listings at once, but better practice is using paginated listings. It's unlikely that a single user has that many listings that fetching all at once is gonna cause any issues, but we never know. Take a look at how this is handled in the main grid view. If you want feel free to take a stab at it here as well, but its not necessary at this pt imo
jamesdoh0109
left a comment
jamesdoh0109
left a comment
There was a problem hiding this comment.
Looks good, i would just maybe change the color of "See all my listings" and "See all saved posts" from red to something else. It looks like a danger button. Also see if you can make the size of the listing card a bit bigger (right now it looks too small) without affecting the size in the main grid view
3 participants
Created User Dashboard with listings and saved posts.
