fix: resolve all outstanding review feedback in #541 #542
Conversation
Create shared BrutalistCard component to eliminate duplicate brutalist styling across AuthWall and VibesPanel components. Changes: - Add BrutalistCard component with variants (default, success, error, warning) and sizes (sm, md, lg) - Refactor AuthWall to use BrutalistCard instead of inline styles - Refactor VibesPanel email input and status box to use BrutalistCard - Export BrutalistCard from use-vibes/base Benefits: - Eliminates ~60 lines of duplicate styling code - Single source of truth for brutalist card aesthetic - Consistent styling across all auth UI components - Easier to maintain and extend 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ow consistency; narrow transitions; barrel export\n\n- Type-safe props: extend React.HTMLAttributes<HTMLDivElement>; remove index signature\n- Forward refs from BrutalistCard for better composability\n- A11y: stop suppressing input outline in VibesPanel to restore focus visibility\n- Styles: normalize lg box-shadow format; restrict transition to box-shadow, transform\n- Exports: re-export component from barrel to keep paths uniform
✅ Deploy Preview for fireproof-ai-builder canceled.
|
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall the refactor is solid: shared styling via BrutalistCard reduces duplication and improves consistency, and the a11y fix for input focus is welcome. The main concern is a brittle check in getBoxShadow that relies on a magic color string; compare the variant instead to avoid hidden coupling. Consider importing BrutalistCard via the local barrel for consistency and easier refactors. No functional regressions are evident in the diff.
Additional notes (3)
-
Maintainability |
use-vibes/base/components/AuthWall/AuthWall.tsx:13-13
You added a local barrel (components/BrutalistCard/index.ts) and also re-export fromuse-vibes/base/index.ts, but this file imports the component directly from the implementation file. For consistency and easier refactors, import from the local barrel instead of the deep path. -
Maintainability |
use-vibes/base/components/VibesPanel/VibesPanel.tsx:3-3
Same note as inAuthWall: now that a barrel exists forBrutalistCard, favor importing from the barrel rather than the deep file path to keep imports uniform and make future file reorganizations easier. -
Readability |
use-vibes/base/components/VibesPanel/VibesPanel.tsx:165-174
Good move removing the outline suppression. Given the thick card border, the default outline can still be a bit tight against the input edge in some browsers. A smalloutlineOffsetimproves focus visibility and usability, especially for keyboard users.
Summary of changes
Summary of changes
- Introduced a shared
BrutalistCardcomponent with variants (default,success,error,warning) and sizes (sm,md,lg). - Replaced inline brutalist styles in
AuthWallandVibesPanelwithBrutalistCardusages. - Removed
getFormContainerStylefromAuthWall.styles.tsand adjustedAuthWallto use<BrutalistCard size="lg" />with supplemental layout styles. - Improved a11y by restoring input focus visibility in
VibesPaneland limiting transitions tobox-shadowandtransform. - Added barrel exports for
BrutalistCardinuse-vibes/base/index.tsand a localcomponents/BrutalistCard/index.ts. - Minor style normalization for box shadows and style object annotations.
| case 'md': | ||
| return `4px 5px 0px 0px ${color}`; | ||
| case 'lg': | ||
| return `6px 6px 0px 0px ${color === '#666' ? '#1a1a1a' : color}`; // lg uses black for default |
There was a problem hiding this comment.
The lg branch couples behavior to the magic value '#666' returned by getShadowColor('default'). If the default color changes, this logic silently breaks. Prefer checking variant === 'default' directly so the intent is explicit and resilient to future refactors.
Suggestion
Consider decoupling from the magic color string by checking the variant explicitly:
case 'lg': {
const effectiveColor = variant === 'default' ? '#1a1a1a' : color;
return `6px 6px 0px 0px ${effectiveColor}`;
}
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| fontSize: getFontSize(size), | ||
| fontWeight: 500, | ||
| letterSpacing: '0.02em', |
There was a problem hiding this comment.
BrutalistCard currently sets fontSize, fontWeight, and letterSpacing at the container level. Baking typography into a shared layout container can unintentionally bleed styles into children (e.g., the AuthWall description text) and diverges from pre-refactor behavior where typography was controlled by the child elements. Keeping typography out of layout primitives improves reusability and reduces surprises.
Suggestion
Recommend removing fontWeight and letterSpacing from the card style and letting children define their own typography. Keep fontSize if you still want size-based text scaling, or remove it as well if you prefer pure layout semantics. Then, explicitly set typography where needed (e.g., the VibesPanel email input) to preserve prior appearance.
BrutalistCard.styles.ts:
export function getBrutalistCardStyle(variant: BrutalistCardVariant = 'default', size: BrutalistCardSize = 'md'): CSSProperties {
return {
background: '#fff',
color: '#1a1a1a',
border: '3px solid #1a1a1a',
borderRadius: '12px',
padding: getPadding(size),
fontSize: getFontSize(size), // optionally keep or remove
// fontWeight: 500, // remove
// letterSpacing: '0.02em', // remove
boxShadow: getBoxShadow(size, variant),
transition: 'box-shadow 0.15s ease, transform 0.15s ease',
boxSizing: 'border-box',
};
}VibesPanel.tsx (input style) — add explicit typography to maintain previous look if you remove it from the card:
style={{
width: '100%',
border: 'none',
background: 'transparent',
color: 'inherit',
fontSize: 'inherit',
fontWeight: 500,
letterSpacing: '0.02em',
padding: 0,
}}Reply with "@CharlieHelps yes please" if you'd like me to prepare a commit that applies these changes across both files.
Context
What’s included
Testing
Notes
Related