Skip to content

Feature/image optimization - #5

Open
vaibhav-cw wants to merge 3 commits into
feature/atomic-sturcture-componentsfrom
feature/image-optimization
Open

vaibhav-cw wants to merge 3 commits into
feature/atomic-sturcture-componentsfrom
feature/image-optimization

Conversation

@vaibhav-cw

Copy link
Copy Markdown
Collaborator

✅ What does this PR do?

  • Adds a Logo document schema in Sanity and registers it in schema types
  • Fetches logo from CMS and displays it dynamically in the page header
  • Enhances TextImageSection image field by requiring alt text for accessibility and SEO
  • Cleans up unused imports in TextImageSection component
  • Adds safer image handling logic to avoid runtime errors when image is missing
  • Improves CMS reusability by centralizing site branding (logo) in Sanity

✅ 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 the branch and run the project locally
  • In Sanity Studio: Create/update a Logo document with title CMS-SITE and upload an image
  • Ensure images in TextImage sections include alt text
  • Open any dynamic page route
  • Verify:
    • Logo appears in the header
    • Page title still renders correctly
    • TextImage sections render without errors
    • Removing an image in CMS does not break the page

✅ Pull Request standards checklist

  • This branch carries a single responsibility (CMS logo + image accessibility improvements)
  • Conventional naming followed
  • Folder/file names remain clear and structured

✅ Testing checklist

  • Manual local testing completed
  • CMS content tested with and without images
  • No lint errors remain

✅ Definition of Done

  • Code works as expected in multiple CMS data scenarios
  • Lint & formatting applied
  • Dead code removed
  • Branch rebased with base branch

@mergemitra

mergemitra Bot commented Feb 23, 2026

Copy link
Copy Markdown

Change Summary

Introduces 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
File Summary
app/[slug]/page.tsx Fetches CMS logo, logs page title, renders header with safe dynamic Image handling.
components/molecules/TextImageSection/TextImageSection.tsx Removes unused Container import, keeping ResponsiveImage import alignment with requirements.
sanity/schemaTypes/index.ts Adds logo schema import and registers it alongside page sections in schema types.
sanity/schemaTypes/logo.ts Adds new logo document schema capturing title and uploadable image.
sanity/schemaTypes/sections/textImageSection.ts Requires alternative text on text-image section images to ensure accessibility compliance.

Based on 304452f...15e419e

@mergemitra

mergemitra Bot commented Feb 23, 2026

Copy link
Copy Markdown

PR Scorecard

Scores

Communication Quality Code Correctness & Design Quality Test Quality & Coverage Code Readability & Maintainability
Scoring Methodology

Communication Scoring Framework

The overall communication score is a weighted average:

Dimension Weight Evaluates
PR Description Quality 60% Title format (conventional commits) + Description clarity (what changed & why)
PR Size & Scope 25% Appropriate sizing, scope cohesion, and justification for size
Commit Messages 15% Conventional commits format, atomic & descriptive changes

Formula: (Description x 0.6) + (PR Size x 0.25) + (Commits x 0.15)

Code Scoring Framework

The scorecard evaluates code using 3 key reviewer questions:

Reviewer Question Category
Is this the right solution, implemented the right way? Code Correctness
Would this catch bugs if the code broke tomorrow? Test Quality
Can someone new understand and safely modify this in 6 months? Maintainability
PR Communication Notes

Description Quality

  • ✅ Description clearly explains what changed and why, matching the logo + alt-text schema updates
  • ❌ Title is not conventional commits; use e.g. 'feat(sanity): fetch logo and require alt text'
  • ❌ Many checklist items are left unchecked; mark completed items or remove sections not used
  • ❌ Diff adds a console.log in app/[slug]/page.tsx that isn’t mentioned; remove or document it

PR Size & Scope

  • ✅ Small, focused PR (5 files, ~72 LOC) centered on CMS logo + image accessibility
  • ✅ Scope stays cohesive with supporting schema and UI changes for the same feature

Commit Messages

  • ✅ All commits use conventional prefixes (feat/fix) and are easy to map to the diff
  • ✅ Consider adding scopes in future commits (e.g., 'feat(sanity): ...') for quicker filtering

Issue Notes

Code Correctness & Design Quality

  • 🟠 Logo image field has no required validation, so the CMS can publish a logo doc without an image and the header may render without branding; add required validation (and consider alt text) to keep content complete. (sanity/schemaTypes/logo.ts:16)
  • 🟠 LogoDoc doesn’t match LOGO_QUERY (query returns only image but the type includes title), so TypeScript can’t catch missing fields and logo.title would be undefined; align the query projection and the type. (app/[slug]/page.tsx:21)

Test Quality & Coverage

  • 🟠 No automated tests were added for the new logo fetch/rendering and missing-image scenarios, and the PR description doesn’t explain why; add tests (or document the reason) to prevent regressions. (app/[slug]/page.tsx)

Code Readability & Maintainability

  • 🟠 Logo fetch is keyed off hardcoded title == "CMS-SITE", so renaming/duplicating the doc will silently break branding; prefer a singleton _id/settings doc or parameterize the query via config. (app/[slug]/page.tsx:25)
  • 🟠 Page and logo are fetched sequentially, adding avoidable latency to every request; fetch them in parallel (Promise.all) or combine into a single GROQ request. (app/[slug]/page.tsx:65)
  • 🟠 console.log in a server-rendered page can spam production logs and leak info; remove it or replace with structured logging behind a debug flag. (app/[slug]/page.tsx:72)
💬 Minor Issues (Nitpicks)

Code Correctness & Design Quality

  • 💬 Requiring non-empty alt text for every image can reduce a11y for decorative images (which should use empty alt); consider adding a 'decorative' flag or allowing empty alt when appropriate. (sanity/schemaTypes/sections/textImageSection.ts:28)

Code Readability & Maintainability

  • 💬 Schema and field titles use inconsistent capitalization/formatting (e.g., 'logo', 'logo Title'), which hurts Studio readability; use human-friendly titles and run a formatter for consistent indentation. (sanity/schemaTypes/logo.ts:5)
  • 💬 Validation callback uses Rule as a parameter name (PascalCase), which breaks camelCase naming conventions; rename to rule for consistency. (sanity/schemaTypes/logo.ts:13, similar issue exists in sanity/schemaTypes/sections/textImageSection.ts:28)
  • 💬 if (!page) notFound() is duplicated after already handling the null case; remove the redundant check for clarity. (app/[slug]/page.tsx:69)
  • 💬 Logo image alt text is hardcoded to "Logo"; use a more descriptive value (e.g., site name/CMS title) for better accessibility and SEO. (app/[slug]/page.tsx:80)
  • 💬 params is typed as a Promise, but Next.js passes a plain object; typing it correctly avoids confusion and catches misuse. (app/[slug]/page.tsx:13)

Based on 304452f...15e419e

Comment thread app/[slug]/page.tsx
notFound()
}

const logo = await client.fetch<LogoDoc | null>(LOGO_QUERY)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread app/[slug]/page.tsx
if (!page) notFound()

const imageUrl = logo?.image ? urlFor(logo.image).url() : null
console.log('Fetched page data:', page.title)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: 🟠 Major

Remove the console.log from this server component (or gate it behind a debug flag) to avoid noisy production logs.

Comment thread app/[slug]/page.tsx
Comment on lines +21 to +26
type LogoDoc = {
title: string
image: SanityImageSource
}
const LOGO_QUERY = `*[_type == "logo" && title == "CMS-SITE"][0]{
image}`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 })

@mergemitra

mergemitra Bot commented Feb 23, 2026

Copy link
Copy Markdown

PR Overview

PR Type: Feature

Focus Areas for Architect Review

  • Consider modeling branding as a singleton settings/logo doc (fixed _id) and fetching it in a shared layout/header instead of per-page querying by mutable title.
  • Review caching/revalidation for Sanity fetches (page + logo) to avoid extra CMS round trips on every request and ensure consistent data snapshots.
  • Confirm the team’s a11y strategy for “required alt text” covers decorative images and is consistently applied across all image surfaces (including the site logo).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant