fix(cards): drop the award action and match the glass bar sizing on feed cards - #6394
fix(cards): drop the award action and match the glass bar sizing on feed cards#6394tsahimatsliah wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
tsahimatsliah
left a comment
There was a problem hiding this comment.
Summary
Three independently sensible fixes, and the two logic ones are correct:
- Award vs impressions gate. Dropping
|| isLaptopis right, and it's fixed in bothActionButtons.tsxandActionButtons.v2.tsxso the two bars agree. isSameUser. Theundefined === undefineddiagnosis holds:if (!canAward && !isSameUser) return null, so an anonymous visitor on an authorless post fell through totrueand rendered the button. I traced the other quadrants — logged-in + authorless was alreadyfalse, and logged-out on an authored post is governed byuseCanAwardUser, which returnsfalsewhenever there's nosendingUser. So the fix is narrow and touches exactly the broken case, in both variants. (Small correction to the description: logged-out visitors were already not seeing it on authored posts; that half wasn't broken.)CardActionBargap removal is contained —layout="feedCard"has exactly one consumer,ActionButtons.v2.
The sizing change is where I'd push back, on one specific case.
Blocking
The control cohort — card_impressions off — is the one layout that wasn't measured, and it's plausibly the widest. With the flag off, the award action still renders, and PostAwardActionV1 hardcodes size={ButtonSize.Small} — it does not take buttonSize from variantConfig. So a control feed card now shows five XSmall buttons next to one Small one. Three consequences:
- Width. The measurements in the description ("tightest case 272px … 4px gap") are for the impressions-on row, which has five actions. Control has six, one of them at the old, larger size, and it can carry its own
numAwardscounter. Since buttons never shrink (flex-shrink: 0), the surplus becomes overflow rather than compression. The new Storybook page only setscard_impressions: truein all six rows, so this case isn't covered there either. - Alignment. "The two bars now have identical internal geometry" isn't true in control — the award button is a size taller and wider than its neighbours.
- Icon size. The un-awarded glyph takes the passed
iconSize(now 16px), but the awarded state is pinned toiconSizeToClassName[IconSize.XSmall](20px), so the same button changes size when a post gets awarded. Its counter also keepslabelClassName="!pl-[1px]"rather than the new sharedcounterLabelClassName.
The fix looks small — thread buttonSize/density into PostAwardActionV1 the way v2 already does with density="tight", and drop the hardcoded XSmall on the awarded image. Either way, please add a card_impressions: false row to the alignment story and re-check 272px, since that's the cohort the sizing change ships to unconditionally.
Non-blocking
1. The useViewSize mock in ActionButtons.spec.tsx is now vestigial and pins one viewport. It's forced to true (laptop), which is the right regression case for the old gate — but with the gate gone, the test no longer proves viewport-independence, and a reintroduced isLaptop check would still pass. describe.each over [false, true] for the viewport, or dropping the mock entirely, would make the assertion match what the fix actually claims.
2. PostAwardAction.spec.tsx doesn't cover logged-out on an authored post — the other half of the description's claim. It's currently protected by useCanAwardUser rather than by this change, so a case pinning it would document which mechanism owns it.
3. Seven Merge branch 'main' commits. Not worth a round trip on its own, but the history is hard to read against the four real commits.
4. CI was still running when I reviewed (install_deps green, everything else pending) — worth a glance before merge, particularly lint_shared given the useViewSize/classNames import churn.
Verification
- Read the PR review guide and code-style rules
- Traced all four auth/author quadrants through
isSameUser+useCanAwardUserin both variants - Checked every consumer of
layout="feedCard"before accepting the gap removal - Compared
PostAwardActionV1's sizing against the newvariantConfigvalues - Read both new specs and the alignment story's feature overrides
- Not measured on the preview — the 272px control-cohort case above is the specific thing I could not rule out from the diff
- CI incomplete at review time
Verdict
Comment — the control-cohort layout needs either a measurement or the size threaded through before merge; everything else is optional.
Reviewed by AI.
…e on Impressions take the award slot on the right of the engagement bar, so the two never coexist. Awards were only dropped below laptop; now they are gone on every viewport for grid, list and signal cards, in both engagement bar variants. With the card_impressions flag off, cards are unchanged. Also fixes the award button rendering for logged out visitors: `user?.id === post?.author?.id` compared undefined to undefined on authorless posts, so `isSameUser` was true and the button appeared on exactly the cards where an award is impossible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The impressions counter sat 5px from the card edge on the default bar, so the number read as touching the rounded corner. Adopt the sizing the glass pill already uses (FeedCardGlassActions, #6260): XSmall buttons, 16px icons, `!pl-0.5 pr-0.5` around counters and asymmetric `pl-1 pr-2.5` row padding, which gives the two bars identical internal geometry — first icon 9px in, trailing number 13px from the edge, verified at the 272px min card width. Drops the mobile-only `typo-caption1` counter: the count is now footnote everywhere, matching the glass bar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The v2 bar pushed its trailing action up to 41px outside the card on the narrowest grid track. Buttons never shrink here (global flex-shrink: 0), so six actions plus counters have to fit on intrinsic widths alone, and at Small size (32px buttons, 20px icons) they need up to 288px inside a 262px row. Two changes close the gap: - new `tight` CardAction density (XSmall buttons, 16px icons), used by the feed bar only, so v2 matches the v1 and glass bars. - no `gap` on the `feedCard` CardActionBar layout: `justify-between` already spreads the actions when there is slack, and the gap only added 20px the card could not give back. Both default bars now sit at a 36px row height (`py-1.5` around the h-6 buttons) so shrinking the actions doesn't make the bar read as cramped. Verified at 272px and 320px with 36·3·52.4K, 100·80·100K, 200·80·234.5K and 10K·999·1.2M: nothing escapes its container in any combination. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Renders v1 / v2 / glass feed bars at 320px and the 272px min card width across four stat loads, so the width contract stays checkable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ensity The /dev/buttons width-contract vignettes documented `density="compact"` as the grid-card rule, which overflows the 272px MIN once a card carries six actions with counters. They now use `tight`, matching what the feed ships. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review catch: PostAwardActionV1 hardcoded ButtonSize.Small, so the control cohort — card_impressions off, where the award action still renders — got five 24px buttons next to one 32px one, and the surplus could overflow the 272px card rather than compress. - v1 takes buttonSize/iconSize from the caller, defaulting to the shared feed-action sizes; the awarded image follows iconSize instead of a pinned IconSize.XSmall, so the button no longer resizes when a post gets awarded. - v2 defaults to the tight density and derives the awarded image size from it, matching what the feed bar already passes. - Counter classes moved to actionCounter.ts so the award action, both bars and the glass pill can't drift apart. - Alignment story renders the control cohort (award present, with counts) at the 272px min card width for both bars. - ActionButtons spec runs both viewports, so a reintroduced isLaptop gate fails instead of passing on the unmatched-media-query default. - PostAwardAction spec pins the button and awarded-image sizes, and covers logged out on an authored post, which useCanAwardUser owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
75f953c to
48d03e8
Compare
|
Review addressed in 48d03e8, and the branch is rebased onto Blocking — control cohort sizing. You were right that this was the unmeasured case. Blocking — story coverage. Two Measured across all four stat loads at the 272px min card width:
Non-blocking 1 — viewport mock. Non-blocking 2 — logged out on an authored post. Pinned in Non-blocking 3 — merge commits. Rebased away. Non-blocking 4 — CI. Green on the previous push; re-running now. One extraction while I was in there: the counter classes and the two feed-action size constants live in |
…n constants Same values, one source, so the glass pill and the two default bars stay in step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three fixes to the feed card action bar.
1. Impressions and awards competed for the same slot. The gate was
showAwardAction && (!impressionsEnabled || isLaptop), so withcard_impressionson the award action was only dropped below laptop and still rendered on desktop next to the impressions stat. Impressions replace that button, so awards are now hidden on every viewport for all feed card variants (grid, list, signal) in both engagement bar variants.2. The award button leaked onto authorless cards for logged out visitors.
isSameUserwasuser?.id === post?.author?.id— for an anonymous visitor on a post with no author that comparesundefinedtoundefinedand evaluates totrue, so the button rendered on exactly the cards where an award is impossible. Logged out on an authored post was already correct (useCanAwardUserreturnsfalsewithout a sending user); a spec now pins that so it's clear which mechanism owns it.3. The impressions counter sat 5px from the card edge, reading as if it touched the rounded corner. Both default bars now use the sizing the floating glass pill has had since #6260:
XSmallbuttons, 16px icons,!pl-0.5 pr-0.5around counters, and asymmetricpl-1 pr-2.5row padding, withpy-1.5holding the row at its previous 36px height. The trailing number now sits 13px from the edge, and the award action takes the same sizing instead of hardcodingButtonSize.Small.Getting there needed two supporting changes: a
tightCardAction density (compactis untouched, so comments, the reader bar, daily votes and hot takes are unaffected), and droppinggap-1from thefeedCardCardActionBar layout — withjustify-betweena gap is redundant when there is slack and pure added width when there isn't. Without both, the v2 bar pushed its trailing action up to 41px outside the 272px card, because buttons never shrink (globalflex-shrink: 0) and six actions with counters needed up to 288px in a 262px row.With the
card_impressionsflag off, feed cards keep the award action exactly as onmain; the sizing change applies either way. The award action on the post page, reader rail and focus card is untouched.Measurements
Storybook
Components/Cards/ActionBarAlignmentrenders every bar at 320px and the 272px min card width, across 36·3·52.4K, 100·80·100K, 200·80·234.5K and 10K·999·1.2M — including the control cohort, where the award action is present with its own counter.Tests
ActionButtons.spec.tsx— both engagement bar variants x both sides of the laptop breakpoint x grid/list/signal: award absent and impressions present when the flag is on, award present when it is off.PostAwardAction.spec.tsx— both variants: visibility across all four auth/author quadrants, plus the button size and the awarded-image size.🤖 Generated with Claude Code
Preview domain
https://claude-award-button-visibility-c.preview.app.daily.dev