Repository navigation
feature: fetching sponsor of the week - #198
Conversation
joengy
left a comment
joengy
left a comment
There was a problem hiding this comment.
I left some comments for how you're dealing with the database, they are quite detailed but if you need any additional help I'm happy to help however next week I might not have as much time because its systems week for me.
|
Made the changes, let me know if there are any improvements still required. |
JotinderBhamra
left a comment
JotinderBhamra
left a comment
There was a problem hiding this comment.
good job gladwyn looks good, might need big boy joe to check the backend and see there isnt any looping issues tho.
joengy
left a comment
joengy
left a comment
There was a problem hiding this comment.
I left a few comments for you to address Gladwyn, they aren't anything too complex but if you need help just lmk
| }, | ||
| }, | ||
| collections: [Users, Media, Events, EventRegistrations, Sponsors, Execs, Members], | ||
| globals: [SiteSettings], |
There was a problem hiding this comment.
This global needs a migration. Without one, prod won't have the site_settings / site_settings_home_join_paragraphs / site_settings_about_team_paragraphs tables, since we only run prodMigrations there. See the summary comment for how to generate it.
| @@ -0,0 +1,21 @@ | |||
| import { MigrateDownArgs, MigrateUpArgs, sql } from '@payloadcms/db-postgres' | |||
There was a problem hiding this comment.
This looks hand-written. It doesn't have a matching .json snapshot like the other migrations, so Payload's next migrate:create will diff against the event_rsvps snapshot and generate category again. Can you delete this file and regenerate it with migrate:create? It can go in the same migration as site-settings.
| @@ -0,0 +1,28 @@ | |||
| import type { GlobalConfig } from 'payload' | |||
There was a problem hiding this comment.
These aren't registered in payload.config.ts, and SiteSettings seems to have replaced them. Can we delete them?
| @@ -0,0 +1,30 @@ | |||
| import { fetchFromCMS } from '@/lib/api' | |||
There was a problem hiding this comment.
Same here: nothing imports these now that siteSettings.ts exists, so they can go.
| @@ -0,0 +1,48 @@ | |||
| import { getPayload } from 'payload' | |||
There was a problem hiding this comment.
This duplicates cms/seed-site-settings.ts and probably shouldn't be committed. Maybe add .tmp/ to .gitignore?
| if (CMS_URL === undefined) { | ||
| throw new Error('CMS_URL is not defined in the environment variables.') | ||
| return [] |
There was a problem hiding this comment.
This used to throw when CMS_URL was missing, and now it silently returns []. A misconfigured deploy would show an empty sponsors grid with no sign of why. Could we at least console.error here like the catch block below does?
Also, this reads process.env.CMS_URL, but lib/api.ts (used by fetchSiteSettings) reads NEXT_PUBLIC_CMS_URL. If prod only sets one of them, half the pages fall back to defaults. Could fetchSponsors use fetchFromCMS from lib/api.ts so there's a single source?
| data.eventsHeroSubtitle || | ||
| 'Join us for exciting events, cultural celebrations, and community gatherings throughout the year.', | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Two small things:
- Can we log the error here? Right now a broken CMS connection is invisible.
- The default text is duplicated between the success path and this catch block, and it's also in the CMS field defaults, the seed script and the
JoinCard/ExecGriddefault props. Maybe pull it into a singleDEFAULT_SITE_SETTINGSobject and use it in both branches here? Then the components don't need their own defaults either.
| }) | ||
|
|
||
| // Use the CMS event when available; otherwise retain Ice Kachang. | ||
| const event = data?.event ?? fallbackEvent |
There was a problem hiding this comment.
When there's no upcoming event, or the request fails, this shows the April Ice Kachang event as if it's still upcoming. Before, the events page said "No upcoming events right now". Is that intentional? I'd lean towards keeping the "no upcoming events" message so we don't advertise a past event.
| "drizzle-kit": "0.31.10", | ||
| "drizzle-orm": "0.44.7", |
There was a problem hiding this comment.
Are drizzle-kit/drizzle-orm needed at the root? drizzle-kit is already added to cms/package.json, and nothing at the root uses them.
| get involved. | ||
| </p> | ||
| {paragraphs.map((paragraph) => ( | ||
| <p key={paragraph}>{paragraph}</p> |
There was a problem hiding this comment.
key={paragraph} will clash if two paragraphs have the same text. Using the index as the key is fine here.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
An empty (or 0) event price now means a free event: registration is confirmed straight away without Stripe, the card shows "Free", and the success page has a free-registration state.
What does this PR do?
fetching sponsors. Overwrites upcoming events
Type of change
Checklist
feature/,fix/,chore/,hotfix/)Linked Issues
Closes #
Addresses #
Related to #