fix(gift-cards): follow standard header pattern across all routes#1074
Draft
gudnuf wants to merge 2 commits into
Draft
fix(gift-cards): follow standard header pattern across all routes#1074gudnuf wants to merge 2 commits into
gudnuf wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
gudnuf
force-pushed
the
fix/page-header-x-shift
branch
from
caf6248 to
4e97856
Compare
gudnuf
changed the title
gudnuf
changed the title
gudnuf
force-pushed
the
fix/page-header-x-shift
branch
from
8ff22b3 to
7e34fa7
Compare
gudnuf
force-pushed
the
fix/page-header-x-shift
branch
from
7e34fa7 to
82f0ce6
Compare
gudnuf
force-pushed
the
fix/page-header-x-shift
branch
from
7a9c375 to
0a0e07f
Compare
gudnuf
changed the title
gudnuf
force-pushed
the
fix/page-header-x-shift
branch
from
0a0e07f to
30d9bc8
Compare
gudnuf
force-pushed
the
fix/page-header-x-shift
branch
from
30d9bc8 to
d1c79c8
Compare
Two structural inconsistencies were drifting the X icon between
/gift-cards and its child routes:
1. Detail/offer pages used `absolute inset-x-0` headers (which escape
Page's horizontal padding); list page used in-flow. On desktop Page
has sm:px-6/lg:px-8, so the absolute header's X landed 24-32px left
of the in-flow header's. Switch detail/offer/add to the same in-flow
px-4 pattern as the list.
2. Detail/offer/add used a raw `<button onClick>` for the close action.
<button> is display:inline-block; with only block-svg content (svg is
`display:block` per Tailwind preflight), its baseline is at the
bottom margin edge - the line box grows by the strut's descent and
the X sits a couple px lower than an anchor would. Switch to
ClosePageButton, matching every other page.
Since the cards already define their own view-transition morphs, the
close buttons shouldn't override them with a slide. Make `transition`
optional on LinkWithViewTransition/ClosePageButton - when omitted, the
component renders a plain Link with `viewTransition` for the browser-
default crossfade (same behavior as the previous `navigate('/gift-cards',
{ viewTransition: true })` call).
gudnuf
force-pushed
the
fix/page-header-x-shift
branch
from
d1c79c8 to
7f68e1d
Compare
Drop the `Page className="px-0 pb-0"` + `PageHeader className="px-4"` overrides on all four gift card pages. They were originally introduced to let the list page's DiscoverGiftCards horizontal scroll extend edge-to-edge on mobile, with the header re-adding its own padding to keep the X icon visually offset. But this approach split horizontal padding responsibility between Page and Header asymmetrically — and on desktop, Page's sm:px-6/lg:px-8 still applied on top of the header's px-4, so the X icon sat 16-32px right of where it does on every other page in the app. With default Page and PageHeader, the gift card pages match the standard pattern used everywhere else. X icon position is consistent across all routes without any responsive-padding hacks. Tradeoff: DiscoverGiftCards and OffersSection (3+ mode) no longer extend edge-to-edge on mobile — they sit inside Page's px-4. Inner content sections that had their own `px-4` will also indent more (Page padding stacks on top of theirs). Evaluating visual; may revert.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Every other page in the app uses the same header pattern:
```tsx
<ClosePageButton to="..." transition="..." applyTo="..." />
Title
```
That pattern works because:
The gift card detail/offer/add pages deviated in three ways:
Each deviation introduced a positioning bug we kept chasing.
Fix: just use the standard pattern. Drop the absolute positioning, use `ClosePageButton`, add a `PageHeaderTitle` showing the card/offer name. The three deviations turn into three consistencies.
`ClosePageButton`'s `transition` prop is now optional — omitting it renders a plain `` for the browser-default crossfade. The gift card routes use this mode because the cards already define their own `viewTransitionName` morphs that should not be overridden by a slide.
Visual change: detail/offer card images no longer extend behind the X icon — they start below the in-flow header. The card name now appears as the header title.
Test plan