Skip to content

Conversation

@rebelchris
Copy link
Contributor

@rebelchris rebelchris commented Jan 26, 2026

Summary

  • Unified ActionButtons: Consolidated grid and list ActionButtons into a single component with variant prop, reducing ~150 lines of duplicate code
  • Extracted reusable hooks: Created usePollVote and useCardActions hooks for shared logic across card components
  • Consolidated RaisedLabel: Merged grid and list RaisedLabel implementations into one component with focus prop
  • Added Storybook: New FeedCardsOverview story showing all card types in grid/list layouts with proper grid styling matching the app

Changes

Change Before After
ActionButtons 2 separate implementations (grid + list) 1 unified component with variant prop
Poll voting logic Duplicated in PollGrid + PollList Extracted to usePollVote hook
Card action handlers Inline in ActionButtons Extracted to useCardActions hook
RaisedLabel 2 files (grid + list) 1 component with focus prop
Barrel files ActionsButtons/index.ts Deleted (direct imports)

Test plan

  • View FeedCardsOverview in Storybook at http://localhost:6006
  • Verify all grid cards render correctly in 3-column layout
  • Verify all list cards render correctly
  • Test card interactions (upvote, bookmark, copy link)
  • Verify card states (read, bookmarked, upvoted, trending, pinned)

🤖 Generated with Claude Code

Preview domain

https://refactor-feed-cards-atomic-desig.preview.app.daily.dev

- Unify ActionButtons into single component with `variant` prop (grid/list)
- Extract `usePollVote` hook for shared poll voting logic
- Extract `useCardActions` hook for card action button handlers
- Consolidate RaisedLabel into one component with `focus` prop
- Add `isVideoType` prop to FeedbackGrid for API consistency
- Delete duplicate ActionButtons implementations and barrel files
- Add FeedCardsOverview storybook for visual testing all card variants

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@rebelchris rebelchris requested a review from a team as a code owner January 26, 2026 13:34
@vercel
Copy link

vercel bot commented Jan 26, 2026

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

Project Deployment Review Updated (UTC)
daily-webapp Ready Ready Preview Jan 27, 2026 10:38am
storybook Building Building Preview Jan 27, 2026 10:38am

Request Review

@rebelchris
Copy link
Contributor Author

@claude review this please, ensure this can't break any existing implementations

@claude
Copy link
Contributor

claude bot commented Jan 26, 2026

Claude finished @rebelchris's task in 2m 18s —— View job


Code Review Complete ✅

I've thoroughly reviewed this refactor and it looks excellent. This is a well-executed consolidation that maintains full backward compatibility while significantly reducing code duplication.


Summary

Overall Assessment: ✅ APPROVED - No breaking changes detected

This refactor successfully:

  • Consolidates ~412 lines of duplicate ActionButtons code into 238 lines (47% reduction)
  • Extracts reusable hooks that eliminate duplicate logic
  • Properly handles both grid and list variants through a clean configuration approach
  • Deletes barrel/index files (following CLAUDE.md guidelines)
  • Maintains API compatibility across all 10 card components

Detailed Review

✅ ActionButtons Consolidation (packages/shared/src/components/cards/common/ActionButtons.tsx)

What changed:

  • Merged grid (ActionsButtons/ActionButtons.tsx) and list (common/list/ActionButtons.tsx) implementations
  • Added variant prop ('grid' | 'list') to control behavior
  • Used variantConfig object to handle differences between variants

Analysis:

  • API Compatibility: ✅ All existing props remain the same
  • Behavior Preservation: ✅ Grid/list differences properly handled through config
  • Type Safety: ✅ Strong typing with ActionButtonsVariant type
  • No Breaking Changes: All 10 card components import and use it correctly

Key differences properly handled:

grid: {
  buttonSize: ButtonSize.Small,
  iconSize: IconSize.XSmall,
  showTagsPanel: false,
  useCommentLink: false,  // Button only
}
list: {
  buttonSize: undefined,
  iconSize: IconSize.Medium,
  showTagsPanel: true,
  useCommentLink: true,   // Link with navigation
}

✅ useCardActions Hook (packages/shared/src/hooks/cards/useCardActions.ts)

What changed:

  • Extracted common toggle logic from both ActionButtons implementations
  • Handles upvote, downvote, bookmark, and copy link actions
  • Manages tags panel state for list variant

Analysis:

  • Logic Preservation: ✅ Matches original behavior exactly
  • Edge Case Handling: ✅ Properly handles the list-specific tags panel close on upvote
  • Type Safety: ✅ Well-typed interfaces with clear return types
  • Proper Dependencies: ✅ useCallback dependencies are correct

Critical behavior preserved:

// List variant: close tags panel when switching from downvote to upvote
if (closeTagsPanelOnUpvote && isDownvoteActive && showTagsPanel) {
  onClose(true);
}

✅ usePollVote Hook (packages/shared/src/hooks/post/usePollVote.ts)

What changed:

  • Extracted duplicate poll voting logic from PollGrid and PollList
  • Manages vote casting, animation state, and view tracking

Analysis:

  • Logic Preservation: ✅ Identical to both original implementations
  • State Management: ✅ Properly manages shouldAnimateResults state
  • Type Safety: ✅ Clean interfaces with proper return types
  • Usage: ✅ Both PollGrid and PollList now use this hook consistently

✅ RaisedLabel Consolidation (packages/shared/src/components/cards/common/RaisedLabel.tsx)

What changed:

  • Merged grid RaisedLabel and list RaisedLabel into one component
  • Added listMode and focus props to handle different rendering modes
  • Fixed color token: bg-theme-bg-cabbagebg-raw-cabbage-40 (correct design system token)

Analysis:

  • API Compatibility: ✅ Both APIs preserved (grid uses default, list passes props)
  • Behavior Preservation: ✅ Three rendering modes properly handled:
    1. Grid mode (default): Label above card with CSS animation
    2. List mode without focus: Label to the right with CSS animation
    3. List mode with focus: Simplified inline positioning
  • Visual Consistency: ✅ Original styling maintained

Minor improvement spotted:

// Old: bg-theme-bg-cabbage (non-standard token)
// New: bg-raw-cabbage-40 (correct design system token)

✅ Import Path Changes

What changed:

  • Deleted barrel file: ActionsButtons/index.ts
  • All imports now point to ../common/ActionButtons

Verified imports across all card types:

  • ✅ ArticleGrid & ArticleList
  • ✅ PollGrid & PollList
  • ✅ ShareGrid & ShareList
  • ✅ FreeformGrid & FreeformList
  • ✅ CollectionGrid & CollectionList

All imports updated correctly, following CLAUDE.md guidelines to avoid barrel files.


Potential Issues

⚠️ Minor: RaisedLabel API could be simplified

The RaisedLabel component now has three rendering modes based on listMode and focus props:

// Grid mode
<RaisedLabel type={type} />

// List mode (current codebase doesn't use this)
<RaisedLabel listMode type={type} />

// List mode with focus (used in list cards)
<RaisedLabel listMode focus={isActive} type={type} />

The middle case (listMode without focus prop) appears unused. Consider simplifying to just:

  • Default (grid): no props needed
  • List with focus: listMode={true} and focus={boolean}

Impact: Low priority - not breaking, just slightly confusing API


Testing Recommendations

Based on the changes, focus testing on:

  1. ActionButtons variant behavior:

    • ✅ Grid cards show small buttons without link navigation
    • ✅ List cards show larger buttons with comment link navigation
    • ✅ Tags panel opens on downvote (list only)
    • ✅ Tags panel closes when switching from downvote to upvote (list only)
  2. Poll voting:

    • ✅ Vote casting works in both grid and list cards
    • ✅ Results animate after voting
    • ✅ View post tracking fires correctly
  3. Card states:

    • ✅ Read, bookmarked, upvoted states render correctly
    • ✅ Trending/hot and pinned labels display properly
    • ✅ All interactive states (hover, focus, active) work
  4. Storybook verification:

    • The new FeedCardsOverview story is excellent for visual regression testing
    • Check all card types in both grid and list layouts

Code Quality Notes

Strengths:

  • ✅ Clean separation of concerns (UI, hooks, logic)
  • ✅ Excellent use of TypeScript for type safety
  • ✅ Good documentation in comments
  • ✅ Follows atomic design principles
  • ✅ Adheres to CLAUDE.md guidelines (no barrel files)
  • ✅ Comprehensive Storybook story for testing

Minor suggestions for future:

  • Consider extracting variantConfig to a separate constants file if more variants are added
  • The useCardActions hook has a long dependency array (13 items) - could potentially be optimized with refs for stable callbacks

Conclusion

This refactor is safe to merge. It's a textbook example of good refactoring:

  • Zero breaking changes
  • Significant code reduction
  • Improved maintainability
  • Proper testing infrastructure (Storybook)
  • Follows project conventions

The consolidation eliminates ~150+ lines of duplicate code while maintaining full backward compatibility. All 10 card components (5 grid + 5 list) continue to work as before.

Recommendation: ✅ APPROVE


| PR Branch

@rebelchris rebelchris merged commit 277364c into main Jan 27, 2026
11 checks passed
@rebelchris rebelchris deleted the refactor/feed-cards-atomic-design branch January 27, 2026 10: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.

2 participants