feature/sponsor-page01#75
Conversation
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Good stuff! Just a small comment on the UI
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for making these changes so quick :D
I'll make a follow up ticket to make the event/sponsor cards responsive (to match the Figma) - as on mobile views there will be no image.
joengy
left a comment
joengy
left a comment
There was a problem hiding this comment.
Nice work on the sponsor page is coming together well, and the HighlightCard badge-variant generalization is a clean change that the home and events pages benefit from too.
Theres a few changes you need to make and the page should make, and I added some suggestions worth considering but they don't have to be implemented.
Also the description body is empty for this pr, so you'll need to fill it out and the branch name feature/sponser-event-card has a typo in "sponsor".
There was a problem hiding this comment.
This looks like an accidental npm install in web/ the repo uses pnpm (pnpm-lock.yaml + pnpm-workspace.yaml at the root) and the only entries here are platform-specific @next/swc-* binaries. Can we delete this file? Re-run pnpm install at the repo root if anything actually needed updating.
| const sponsorEntries: SponsorGridItem[] = Array.from( | ||
| { length: 25 }, | ||
| (_, index) => { | ||
| const sponsor = sponsorSeedEntries[index % sponsorSeedEntries.length] | ||
|
|
||
| return { | ||
| ...sponsor, | ||
| id: index + 10, | ||
| } | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Another suggestion if you don't want to render the sponsors like how we talked about before could be rendering sponsorSeedEntries directly
So it would look like
const sponsorEntries: SponsorGridItem[] = sponsorSeedEntries
If you want the page to look more populated for design review, add more distinct seed entries to sponsorSeedEntries instead of repeating them. When the CMS endopoints are done, that line becomes await fetchSponsors() and the grid grows naturally rather than fixing the length of the sponsor array to 25.
| websiteUrl={sponsor.websiteUrl ?? '/sponsors'} | ||
| hoverOverlayClassName={sponsor.hoverOverlayClassName} | ||
| hoverTitle={sponsor.name} | ||
| hoverDescription={sponsor.memberPerks ?? ''} |
There was a problem hiding this comment.
hoverDescription={sponsor.memberPerks ?? ''}
will render the hover overlay with the left vertical divider line and an empty text block when a sponsor has no perks. Maybe consider blocking entirely when empty, or falling back to something like "Visit website". But realistically I don't think there will be a sponsor without a perk, this is just a really random edge case.
| return ( | ||
| <a | ||
| href={websiteUrl} | ||
| target="_blank" |
There was a problem hiding this comment.
<a target="_blank"> is correct for the external sponsor links, but SponsorsGrid passes '/sponsors' as a fallback when websiteUrl is missing, and that fallback would open the current site in a new tab, which is a bit weird. Consider rendering a
| function getBadgeText(badge: HighlightCardBadge) { | ||
| return typeof badge === 'string' ? badge : badge.text | ||
| } | ||
|
|
||
| function getBadgeClassName(badge: HighlightCardBadge) { | ||
| const variant = typeof badge === 'string' ? 'red' : (badge.variant ?? 'red') | ||
| const variantClassName = | ||
| variant === 'light' | ||
| ? 'bg-[#FCE6BA] text-[#706F6F]' | ||
| : 'bg-ssa-red text-ssa-yellow-light' |
There was a problem hiding this comment.
The new badge variants use hard-coded hex (bg-[#FCE6BA], text-[#706F6F]). If these colours are reused, add them globals.css for consistency or if theres a similar colour that is already set maybe consider changing that colour to be this new hex code as the designs have changed week by week/use the pre-defined colour instead.
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Looks awesome thanks Jotinder!
One small thing, could you remove the .DS_Store file from the PR? This is an auto-generated macOS file that shouldn't be committed. Not your fault at all, it should've been in our .gitignore from the start.
@joengy could you add .DS_Store to .gitignore in a follow-up PR? Actually I'm looking at our current .gitignore and it looks pretty bare, could you search up a standard React app .gitignore and replacing it.
But that's a job for Joe. For now, just delete the .DS_Store and we're good to merge. Thanks! :D
joengy
left a comment
joengy
left a comment
There was a problem hiding this comment.
Great work overall on all the sponsor stuff, I left a small comment which should be fast to fix, otherwise it looks ready to merge to me.
| ) | ||
|
|
||
| export default async function SponsorsPage() { | ||
| const sponsorOfTheWeek = sponsorOfTheWeekEntry |
There was a problem hiding this comment.
you're creating sponsorOfTheWeek but it's just a copy of sponsorOfTheWeekEntry. You can remove that line and use sponsorOfTheWeekEntry directly instead.
…light card photo removal to match figma
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Looks really good! Thanks for making these changes Jotinder :)
joengy
left a comment
joengy
left a comment
There was a problem hiding this comment.
Good work on the sponsor page, everything LGTM.
3 participants
What does this PR do?
Type of change
Checklist
feature/,fix/,chore/,hotfix/)Linked Issues
Closes #45 AND #46
Addresses #
Related to #