Skip to content

fix: emoji picker and page icon design spec violations (#186)#197

Merged
zacharias-ona merged 4 commits into
mainfrom
fix/186-emoji-picker-design-spec
Apr 17, 2026
Merged

fix: emoji picker and page icon design spec violations (#186)#197
zacharias-ona merged 4 commits into
mainfrom
fix/186-emoji-picker-design-spec

Conversation

@zacharias-ona
Copy link
Copy Markdown
Collaborator

Closes #186

What

The emoji picker and page icon components introduced in PR #185 had four design spec violations: emoji buttons and page icon buttons had touch targets below the 44px mobile minimum, the filter input was missing its border and focus ring, the "Add icon" button was invisible on mobile (hover-only opacity), and the emoji picker clipped on narrow mobile viewports.

How

Applied responsive Tailwind classes to enforce 44px minimum touch targets on mobile while preserving desktop sizes. Added border border-white/[0.06] and focus:ring-2 focus:ring-ring to the filter input per the input spec. Made the "Add icon" button visible on mobile with max-sm:opacity-100. Changed the picker width from fixed w-72 to w-[calc(100vw-16px)] sm:w-72 so it fits within narrow viewports.

Testing

Added 7 static analysis regression tests in src/components/emoji-picker-design-spec.test.ts that verify:

  • Emoji buttons have 44px minimum touch targets
  • Filter input has border and focus ring classes
  • Picker uses responsive width to avoid mobile clipping
  • Page icon button has 44px minimum touch targets
  • "Add icon" button is visible on mobile (not hover-only)

All 220 tests pass. Lint and typecheck clean.

- Emoji buttons: add 44px minimum touch targets on mobile (sm:min-h-8 on desktop)
- Page icon button: add 44px minimum touch targets on mobile
- Filter input: add border border-white/[0.06] and focus:ring-2 focus:ring-ring
- Page icon "Add icon" button: show on mobile with max-sm:opacity-100
- Emoji picker: use viewport-relative width on mobile to prevent clipping
- Add static analysis regression tests for all four violations

Co-authored-by: Ona <no-reply@ona.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
memo Ready Ready Preview, Comment Apr 17, 2026 6:57pm

Request Review

@zacharias-ona
Copy link
Copy Markdown
Collaborator Author

[ci-fix] This PR has failed CI 3+ times. Needs human investigation.

Root cause: The staging job (supabase db reset --linked --yes) fails because the migration 20260417165531_create_page_images_bucket.sql on this branch uses a non-idempotent INSERT — the page-images bucket already exists in the staging Supabase project.

This branch has the old version of the migration (plain INSERT and CREATE POLICY without conflict handling). main was updated in 5b88785 to use ON CONFLICT (id) DO NOTHING and DO $$ BEGIN ... EXCEPTION WHEN duplicate_object wrappers.

Fix: Rebase the branch onto current main, or update supabase/migrations/20260417165531_create_page_images_bucket.sql on this branch to match the idempotent version on main.

@zacharias-ona
Copy link
Copy Markdown
Collaborator Author

[shepherd:conflict] Rebased on main but checks fail. The rebase resolves the merge conflict in src/components/page-icon.tsx (additive — mobile visibility classes), but a design spec test added on main (src/components/page-icon-design-spec.test.ts from issue #191) asserts that the hover-reveal wrapper must NOT use transition-opacity. This PR intentionally adds transition-opacity for the mobile visibility fix.

This is a genuine semantic conflict between two design decisions:

A human needs to decide which behavior is correct, or update the test to allow transition-opacity on mobile.

Conflicting files: src/components/page-icon.tsx, src/components/page-icon-design-spec.test.ts

@zacharias-ona zacharias-ona added the needs-human Needs human input — automation will re-queue when user responds label Apr 17, 2026
Comment thread src/components/emoji-picker.tsx
@zacharias-ona
Copy link
Copy Markdown
Collaborator Author

Review: 1 blocking issue found

The PR correctly addresses all four violations from issue #186 — touch targets, input styling, mobile visibility, and responsive picker width.

Blocking:

  1. Emoji grid overflows on mobile: The grid remains grid-cols-8 but emoji buttons now have min-w-[44px]. On a 375px viewport, 8 × 44px exceeds the available picker width. The grid needs responsive columns (e.g., grid-cols-6 sm:grid-cols-8). See inline comment.

Pushing a fix for this now.

The emoji grid used grid-cols-8 with min-w-[44px] touch targets, causing
overflow on 375px viewports (8×44px = 352px exceeds available width).
Changed to grid-cols-6 on mobile, grid-cols-8 on sm+ breakpoint.

Co-authored-by: Ona <no-reply@ona.com>
The page-icon hover-reveal wrapper had transition-opacity which violates
the design spec requirement for instant hover states. The static analysis
test correctly caught this. Removed the class to make the opacity change
instant.

Co-authored-by: Ona <no-reply@ona.com>
@zacharias-ona zacharias-ona merged commit ca098ff into main Apr 17, 2026
6 checks passed
@zacharias-ona zacharias-ona deleted the fix/186-emoji-picker-design-spec branch April 17, 2026 19:02
@zacharias-ona
Copy link
Copy Markdown
Collaborator Author

⚠️ UI verification found design spec violations. See #201.

Static analysis: 3 instances of arbitrary spacing values (min-h-[44px], min-w-[44px]) where Tailwind scale equivalents exist (min-h-11, min-w-11). The design spec prohibits arbitrary spacing when a scale class matches.

Visual verification: ✅ No visual regressions. Emoji picker renders correctly on desktop (8-col grid) and mobile (6-col grid, full-width). Filter input has border and focus ring. "Add icon" button visible on mobile. Touch targets adequate. No clipping or overflow.

@zacharias-ona
Copy link
Copy Markdown
Collaborator Author

✅ Post-merge verification passed.

E2E suite: 46/47 passed. The 1 failure (search with no matches shows empty state) is a known pre-existing issue (#144), unrelated to this PR's emoji picker/page icon changes.

Ad-hoc smoke tests (all passed):

  • Landing page — loaded, title present
  • /sign-in — rendered with email input
  • /api/health — healthy
  • Authenticated flow — login succeeded, workspace loaded
  • No console errors (unauthenticated or authenticated)

Skipped:

  • /dashboard (not yet built)
  • Editor page navigation (no page buttons found in test workspace)

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

Labels

needs-human Needs human input — automation will re-queue when user responds

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: UI does not match design spec after PR #185

1 participant