MetaMask / MetaMask/metamask-design-system

Clarify styling rules: twClassName (static) vs style with tw.style() (dynamic)

Open
#973 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

documentation enhancement react-native
Dominant language
TypeScript
Stars
37
Forks
14
Avg merge
1d 9h
Merged PRs (30d)
60

Description

Background

Current .cursor/rules/styling.md states:

NEVER store classes in variables - Breaks VSCode Tailwind IntelliSense and linting

However, this rule is ambiguous and the codebase has inconsistent patterns. We need clear guidelines on when to use twClassName vs style with tw.style() in React Native components.

The Problem

What's currently unclear:
  1. When should components use twClassName prop?
  2. When should components use style prop with tw.style()?
  3. Are map lookups (const objects) considered "storing in variables"?
  4. Is storing merged/concatenated class strings acceptable?

Proposed Clarification

✅ twClassName - Pure static strings only

Use twClassName when passing static, unchanging Tailwind classes:

<Box twClassName="rounded-sm border-2 px-4">
  <Text>Static styling</Text>
</Box>
✅ style with tw.style() - Dynamic/conditional classes

Use style array pattern with tw.style() when you need:

  1. Conditionals (pressed, isActive, isDanger, etc.)
  2. Map lookups (severity → color mapping)
  3. Merging with user's style prop
// Good: Map lookup (const object is fine)
const borderColorClass = MAP_BANNER_ALERT_SEVERITY_BORDER_COLOR[severity];

return (
  <BannerBase
    style={[
      tw.style(`border-l-4 ${borderColorClass}`),
      style,  // User's custom style
    ]}
    {...props}
  />
);
❌ Don't store merged/concatenated results

Avoid storing the result of string concatenation/merging:

// ❌ Bad - Stores merged result
const mergedCloseButtonTwClassName = closeButtonTwClassName
  ? `ml-3 ${closeButtonTwClassName}`
  : 'ml-3';

// ✅ Good - Inline in tw.style()
style={[tw.style('ml-3', closeButtonTwClassName), style]}

Components Needing Review

Found these components storing merged/conditional class strings in const variables:

1. BannerBase.tsx (lines 66-68, 77)
// Line 66-68: Stores merged className
const mergedCloseButtonTwClassName = closeButtonTwClassName
  ? `ml-3 ${closeButtonTwClassName}`
  : 'ml-3';

// Line 77: Inline concatenation in prop
twClassName={twClassName ? `rounded-sm ${twClassName}` : 'rounded-sm'}

Suggested fix:

// Use style array pattern
style={[tw.style('ml-3', closeButtonTwClassName), style]}
2. ButtonIcon.tsx (lines 40-46)
// Stores conditional class assignments
const twIconColorClassNames =
  variant === ButtonIconVariant.Floating
    ? 'text-primary-inverse'
    : 'text-icon-default';

const borderRadiusClass =
  variant === ButtonIconVariant.Default ? 'rounded-lg' : 'rounded-full';

Suggested fix:

// Inline in tw.style()
tw.style(
  variant === ButtonIconVariant.Floating 
    ? 'text-primary-inverse' 
    : 'text-icon-default',
  variant === ButtonIconVariant.Default 
    ? 'rounded-lg' 
    : 'rounded-full',
)
3. HeaderBase.tsx (lines 109-112)
// Stores merged base + user className
const baseStyles = 'flex-row items-center gap-4 h-14';
const resolvedTwClassName = twClassName
  ? `${baseStyles} ${twClassName}`
  : baseStyles;

Suggested fix:

// Use tw.style() in array
style={[
  tw.style('flex-row items-center gap-4 h-14', twClassName),
  includesTopInset && { marginTop: insets.top },
  style,
]}
4. Checkbox.tsx (lines 80-84)
// Stores tw.style() result
const twContainerClassNames = tw.style(
  'flex-row items-center',
  isDisabled ? 'opacity-50' : 'opacity-100',
  twClassName,
);

Note: This stores the computed style object, not a class string. Less clear if problematic, but could be inlined.

Reference Implementation

See BannerAlert component (PR #XXX) for the recommended pattern:

  • Uses map lookup for dynamic border color (✅ acceptable)
  • Uses style={[tw.style(...), style]} array pattern
  • Keeps classes inline in tw.style() call
  • Passes twClassName through {...props} to Box

Proposed Rule Updates

Update .cursor/rules/styling.md React Native section with:

  1. Clear distinction:

    • twClassName = static strings only
    • style + tw.style() = dynamic/conditional/merged
  2. Map lookups are fine:

    • Const object lookups (MAP_SEVERITY_BORDER) are NOT "storing in variables"
    • This is a lookup table pattern, not the anti-pattern
  3. Avoid storing merged results:

    • Don't store concatenated/merged class strings in const
    • Keep classes inline in tw.style() for IntelliSense
  4. Array pattern for style merging:

    • Use style={[tw.style(...), style]} when merging with user style
    • Matches Box component foundational pattern

Acceptance Criteria

  • Update .cursor/rules/styling.md with clarified guidelines
  • Refactor BannerBase (lines 66-68, 77)
  • Refactor ButtonIcon (lines 40-46)
  • Refactor HeaderBase (lines 109-112)
  • Review Checkbox (lines 80-84) - decide if acceptable
  • Add examples showing correct patterns
  • Verify all changes maintain 100% test coverage

References

  • Current rule: .cursor/rules/styling.md
  • BannerAlert refactor: banner-alert branch
  • Research findings: See plan mode conversation for 5 pattern analysis

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with .cursor/rules/styling.md and compare its current rule with BannerBase.tsx, ButtonIcon.tsx, HeaderBase.tsx, and Checkbox.tsx. Review the listed class and style patterns, then update the rule and refactor the specified components while deciding whether Checkbox's computed style is acceptable. Done means the acceptance criteria are met and the existing test suite retains 100% coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
react-native, typescript
Domain
documentation, mobile-dev
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.