Skip to content

`refactor: move search to nav, relocate ReviewLayout to features/revi…#5508

Merged
fshowalter merged 4 commits intomainfrom
draft
Mar 22, 2026
Merged

`refactor: move search to nav, relocate ReviewLayout to features/revi…#5508
fshowalter merged 4 commits intomainfrom
draft

Conversation

@fshowalter
Copy link
Owner

…ew, and fix CD deploy timeout`

@claude
Copy link

claude bot commented Mar 15, 2026

Claude finished @fshowalter's task in 1m 27s —— View job


PR Review

  • Gather context and read AGENTS.md
  • Review changed files
  • Post feedback

This is a clean refactor with good improvements. A few observations:

CD Workflow fix (cd.yml) — The timeout-minutes was previously inside the env: block where it had no effect. Moving it to the step level is the correct fix. Switching to npm ci is also the right call for reproducible CI installs.

NavDrawer transition changetransition-[translate,opacity,overlay,display]transition-all in NavDrawer.astro:37. The original was intentionally explicit about animating overlay and display (discrete CSS properties required for <dialog> open/close with @starting-style). transition-all should still work alongside transition-discrete, but it's less precise and slightly less performant (browser tracks all properties). This is a minor nit—the dialog will still animate—but the explicit list was more intentional.

Logo.astro empty frontmatter — After removing the props, the frontmatter fence is left empty with a blank line:

---

---

Valid Astro syntax, but the blank frontmatter could be dropped entirely for cleanliness if the component has no frontmatter needs.

Search <li> in SiteHierarchy — The hardcoded <li> at SiteHierarchy.astro:116-123 duplicates the class list from the dynamically generated <li> items above. Minor, but if the nav item styles ever change, this will need updating in two places.

Everything else looks great:

  • Parameters<typeof fromJsx>[0] in componentToImageResponse.ts is more accurate than ReactNode since it removes the dependency on React's type for what is a library-specific parameter.
  • readingLogEntrysreadingLogEntries typo fix.
  • Removing excerptPlainText and more fields from the reviews schema is clean dead-code removal.
  • Moving ReviewLayout to features/review/ correctly co-locates feature-specific layout with its feature.
  • Consistent className (singular) variable naming across components is a good cleanup.
  • GitHub Actions reporter in vitest.config.ts will improve CI test output.

@fshowalter fshowalter enabled auto-merge March 15, 2026 01:50
@github-actions
Copy link

github-actions bot commented Mar 15, 2026

@github-actions github-actions bot temporarily deployed to pull request March 15, 2026 01:55 Inactive
@fshowalter fshowalter force-pushed the draft branch 2 times, most recently from 42e0ed3 to 20f1482 Compare March 22, 2026 19:14
@github-actions github-actions bot temporarily deployed to pull request March 22, 2026 19:19 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 22, 2026 19:33 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 22, 2026 19:57 Inactive
@fshowalter fshowalter merged commit b2eec6c into main Mar 22, 2026
10 checks passed
@fshowalter fshowalter deleted the draft branch March 22, 2026 19:57
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