Skip to content

fix(cards): free the impressions slot and fit the feed action bar to the min card width - #6394

Merged
tsahimatsliah merged 4 commits into
mainfrom
claude/award-button-visibility-c29508
Aug 24, 2026
Merged

fix(cards): free the impressions slot and fit the feed action bar to the min card width#6394
tsahimatsliah merged 4 commits into
mainfrom
claude/award-button-visibility-c29508

Conversation

@tsahimatsliah

@tsahimatsliah tsahimatsliah commented Jul 30, 2026

Copy link
Copy Markdown
Member

Two things go wrong on the default feed card once the impressions stat is on, plus one award-button bug that predates it.

1. Impressions and awards competed for the same slot. The gate was showAwardAction && (!impressionsEnabled || isLaptop), so awards were 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 grid, list and signal cards in both engagement bar variants.

2. The bar did not fit the card. The impressions counter sat 5px from the card edge at 320px and 6px past it at the 272px minimum; the v2 bar pushed its trailing action up to 41px outside the card. Buttons never shrink here (global flex-shrink: 0), so six actions with counters have to fit on their intrinsic widths alone, and at the old size they needed up to 288px in a 262px row.

Both bars now use 24px buttons with 16px icons, py-1.5 keeping the row at its previous 36px height, and asymmetric pl-1 pr-2.5 so the trailing number is not flush with the rounded corner. Two supporting changes: a tight CardAction density (compact is untouched, so comments, the reader bar, daily votes and hot takes are unaffected), and dropping gap-1 from the feedCard CardActionBar layout — with justify-between a gap is redundant when there is slack and pure added width when there is not.

3. The award button rendered where an award is impossible. isSameUser compared user?.id to post?.author?.id, so an anonymous visitor on an authorless post compared undefined to undefined and got true. Logged out on an authored post was already correct — useCanAwardUser returns false without a sending user — and a spec now pins that so it is clear which mechanism owns it. The action also stops hardcoding ButtonSize.Small, so it matches its neighbours, and its awarded image follows the icon size instead of a pinned IconSize.XSmall.

With card_impressions off, the award action still renders — now at the same size as the rest of the bar.

Measurements

Storybook Components/Cards/ActionBarBeforeAfter puts main's bar next to this branch's; main's side is plain markup so the comparison does not drift when the real component changes.

before (main) after
impressions on, 320px 32px buttons, 20px icons, number 5px from edge 24px, 16px, 13px
impressions on, 272px number 6px outside the card 13px inside
control (award shown), 272px uniformly 32px, 3px between actions uniformly 24px, 10px
v2, impressions on, 272px trailing action 41px outside 11px inside
v2, control, 272px 14px outside 11px inside

Components/Cards/ActionBarAlignment renders the after-state across 36·3·52.4K, 100·80·100K, 200·80·234.5K and 10K·999·1.2M at both widths, impressions on and off. Every row: 6 actions, uniform 24px, nothing escapes the card, worst gap 1px.

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.

Rebased onto main after the feed_card_glass_actions experiment was removed in #6470; nothing here touches that code path.

🤖 Generated with Claude Code

Preview domain

https://claude-award-button-visibility-c.preview.app.daily.dev

@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
daily-webapp Ready Ready Preview Aug 24, 2026 12:57pm
storybook Building Building Preview Aug 24, 2026 12:57pm

Request Review

@tsahimatsliah tsahimatsliah changed the title fix(cards): drop the award action from feed cards when impressions are on fix(cards): drop the award action and match the glass bar sizing on feed cards Jul 30, 2026
@tsahimatsliah
tsahimatsliah requested a review from idoshamun July 30, 2026 11:05

@tsahimatsliah tsahimatsliah left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Summary

Three independently sensible fixes, and the two logic ones are correct:

  • Award vs impressions gate. Dropping || isLaptop is right, and it's fixed in both ActionButtons.tsx and ActionButtons.v2.tsx so the two bars agree.
  • isSameUser. The undefined === undefined diagnosis holds: if (!canAward && !isSameUser) return null, so an anonymous visitor on an authorless post fell through to true and rendered the button. I traced the other quadrants — logged-in + authorless was already false, and logged-out on an authored post is governed by useCanAwardUser, which returns false whenever there's no sendingUser. 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.)
  • CardActionBar gap 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:

  1. 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 numAwards counter. Since buttons never shrink (flex-shrink: 0), the surplus becomes overflow rather than compression. The new Storybook page only sets card_impressions: true in all six rows, so this case isn't covered there either.
  2. 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.
  3. Icon size. The un-awarded glyph takes the passed iconSize (now 16px), but the awarded state is pinned to iconSizeToClassName[IconSize.XSmall] (20px), so the same button changes size when a post gets awarded. Its counter also keeps labelClassName="!pl-[1px]" rather than the new shared counterLabelClassName.

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 + useCanAwardUser in both variants
  • Checked every consumer of layout="feedCard" before accepting the gap removal
  • Compared PostAwardActionV1's sizing against the new variantConfig values
  • 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.

Comment thread packages/shared/src/components/cards/common/ActionButtons.tsx
Comment thread packages/shared/src/components/cards/common/ActionButtons.spec.tsx Outdated
@tsahimatsliah

Copy link
Copy Markdown
Member Author

Review addressed in 48d03e8, and the branch is rebased onto main so the history is the six real commits with the merge commits gone.

Blocking — control cohort sizing. You were right that this was the unmeasured case. PostAwardActionV1 hardcoded ButtonSize.Small, so with card_impressions off a card showed five 24px buttons next to one 32px one. It now takes buttonSize/iconSize from the caller, defaulting to shared feed-action constants. The two follow-ons you spotted are fixed too: the awarded image follows iconSize instead of a pinned IconSize.XSmall (the button used to change size when a post got awarded), and the counter uses the shared label/counter classes rather than !pl-[1px]. v2 had the same pin — it now defaults to the tight density and derives the image size from it.

Blocking — story coverage. Two card_impressions: false rows at 272px, one per bar. The mock post carries an author matching the mocked boot user and a per-case numAwards, so the control rows render the award action with its counter — the widest layout on the page.

Measured across all four stat loads at the 272px min card width:

bar @272px actions button height escapes card worst gap
v1, impressions on 6 24px never (11px inside) 4px
v2, impressions on 6 24px never 1px
v1, control (award) 6 24px never 7px
v2, control (award) 6 24px never 3px
glass 6 24px never (inside pill) 1px

Non-blocking 1 — viewport mock. ActionButtons.spec.tsx is now describe.each over [v2, laptop], 24 cases. A reintroduced isLaptop gate fails on the laptop half instead of passing on jsdom's unmatched-media-query default.

Non-blocking 2 — logged out on an authored post. Pinned in PostAwardAction.spec.tsx with a comment naming useCanAwardUser as the owner. I also corrected the PR description: that half was never broken.

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 cards/common/actionCounter.ts, imported by both bars and the award action, so they can't drift apart again.

tsahimatsliah and others added 3 commits August 24, 2026 15:28
`isSameUser` compared `user?.id` to `post?.author?.id`, so an anonymous
visitor on a post with no author compared `undefined` to `undefined` and got
`true` — the button rendered on exactly the cards where an award is
impossible, and stayed hidden on the ones that have an author. It now
requires a real logged in id.

While here, the action stops hardcoding its own geometry: it takes
`buttonSize`/`iconSize` from the caller so it matches the rest of the feed
bar, and the awarded image follows that icon size instead of a pinned
`IconSize.XSmall`, which used to resize the button once a post was awarded.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The impressions counter sat 5px from the card edge at 320px and 6px past it
at the 272px minimum, and the v2 bar pushed its trailing action up to 41px
outside the card. Buttons never shrink here (global `flex-shrink: 0`), so six
actions with counters have to fit on their intrinsic widths alone, and at the
old size they needed up to 288px in a 262px row.

- both bars move to 24px buttons with 16px icons, `py-1.5` keeping the row at
  its previous 36px height, and asymmetric `pl-1 pr-2.5` so the trailing
  number is not flush with the rounded corner;
- v2 gets a `tight` CardAction density for that; `compact` is untouched, so
  comments, the reader bar, daily votes and hot takes are unaffected;
- the `feedCard` CardActionBar layout drops its `gap`, which `justify-between`
  makes redundant when there is slack and costly when there is not;
- awards give up their slot to the impressions stat on every viewport rather
  than only below laptop, so the two never render side by side.

With `card_impressions` off the award action still renders, now at the same
size as its neighbours.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ActionBarBeforeAfter` puts main's bar next to this branch's at 320px and the
272px minimum, with impressions on and off, for both engagement bar variants;
main's side is plain markup so the comparison does not drift when the real
component changes. `ActionBarAlignment` renders the after-state across four
stat loads. The /dev/buttons vignettes move to the `tight` density so the
internal review page documents what the feed ships.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tsahimatsliah
tsahimatsliah force-pushed the claude/award-button-visibility-c29508 branch from 0f982f7 to 3be24bd Compare August 24, 2026 12:29
@tsahimatsliah tsahimatsliah changed the title fix(cards): drop the award action and match the glass bar sizing on feed cards fix(cards): free the impressions slot and fit the feed action bar to the min card width Aug 24, 2026
@tsahimatsliah
tsahimatsliah merged commit 1e309d1 into main Aug 24, 2026
13 checks passed
@tsahimatsliah
tsahimatsliah deleted the claude/award-button-visibility-c29508 branch August 24, 2026 13:19
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.

2 participants