Feature/image optimization - #5
vaibhav-cw wants to merge 3 commits into
Conversation
Change SummaryIntroduces CMS-managed logo retrieval and safer header rendering, including a dynamic heading display. Strengthens TextImageSection by enforcing alternative text and trimming unused atom imports. Registers the new logo schema to centralize branding updates via Sanity. File Changes
Based on 304452f...15e419e |
PR ScorecardScoresScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
Issue NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
💬 Minor Issues (Nitpicks)Code Correctness & Design Quality
Code Readability & Maintainability
Based on 304452f...15e419e |
| notFound() | ||
| } | ||
|
|
||
| const logo = await client.fetch<LogoDoc | null>(LOGO_QUERY) |
There was a problem hiding this comment.
Severity: 🟠 Major
Add automated coverage for the new header logo behavior (renders when logo exists, and gracefully omits when missing) to prevent regressions; if testing the server component directly is hard, extract the data-fetching logic into a helper and unit test that.
| if (!page) notFound() | ||
|
|
||
| const imageUrl = logo?.image ? urlFor(logo.image).url() : null | ||
| console.log('Fetched page data:', page.title) |
There was a problem hiding this comment.
Severity: 🟠 Major
Remove the console.log from this server component (or gate it behind a debug flag) to avoid noisy production logs.
| type LogoDoc = { | ||
| title: string | ||
| image: SanityImageSource | ||
| } | ||
| const LOGO_QUERY = `*[_type == "logo" && title == "CMS-SITE"][0]{ | ||
| image}` |
There was a problem hiding this comment.
Severity: 🟠 Major
LogoDoc doesn’t match LOGO_QUERY (and the alt text is currently hardcoded); fetch title alongside image and parameterize the selector so the query, types, and rendering stay aligned.
type LogoDoc = { title: string; image?: SanityImageSource }
const LOGO_TITLE = 'CMS-SITE' as const
const LOGO_QUERY = `*[_type=="logo" && title==$title][0]{title, image}`
const logo = await client.fetch<LogoDoc | null>(LOGO_QUERY, { title: LOGO_TITLE })
PR OverviewPR Type: Feature Focus Areas for Architect Review
|
✅ What does this PR do?
✅ Why these changes?
Alt text mandatory
Ensures accessibility for screen readers, supports WCAG compliance, improves SEO indexing, and provides fallback context when images fail to load.
Logo from CMS instead of hardcoding
Allows non-developers to update branding without code changes and keeps assets reusable across pages.
Safe image handling
Prevents rendering crashes when an image is absent in CMS.
✅ What steps does your reviewer have to take to test this PR manually?
✅ Pull Request standards checklist
✅ Testing checklist
✅ Definition of Done