Lessons Learned: Front-End Code Review Experiences

At first, this blog was written for myself after working on a project with an extremely strict code review process. The client was a large e-commerce website, and they paid attention to every single line of code—to the point where it could feel quite annoying at times. But as the saying goes, no pain, no gain.

After spending enough time on that project, I was deeply influenced by this code review style. I started applying the same standards to my own work and encouraged my team members to do the same. Over time, it proved to be highly effective in preventing issues that could negatively impact code quality, even when those issues seemed very small or insignificant.

Before diving into specific technologies, keep these principles in mind:

• Review the intent, not just the code – Understand what problem the PR is solving.
• Prefer clarity over cleverness – Readable code ages better than smart tricks.
• Be constructive and respectful – Suggest improvements, don’t command them.
• Focus on impact – Prioritize issues that affect users, performance, or maintainability.
• Avoid bike-shedding – Use linters and formatters to reduce style debates.

---

Testing Best Practices

Use Setup Functions for Unit Tests

The Problem: When writing unit tests, it’s tempting to repeat setup code in each test case. This leads to:

• Duplication of boilerplate
• Harder maintenance when setup needs change
• Tests that are harder to read and understand

The Solution: Create a setup function that returns everything your tests need:

// ✅ Good: Centralized setup
function setupTestComponent(props = {}) {
  const defaultProps = {
    label: 'Test Label',
    onClick: jest.fn(),
    ...props
  };
  
  const { result } = renderHook(() => useMyComponent(defaultProps));
  
  return {
    result,
    defaultProps,
    // ... other test utilities
  };
}

describe('MyComponent', () => {
  it('should handle click events', () => {
    const { result, defaultProps } = setupTestComponent();
    // Test implementation
  });
  
  it('should handle different props', () => {
    const { result } = setupTestComponent({ label: 'Custom' });
    // Test implementation
  });
});

Benefits:

• Consistent test setup across all tests
• Easy to modify setup logic in one place
• Tests become more focused on what they’re actually testing
• Reduces test file size and improves readability

Prefer getByRole() Over Other Query Methods

The Problem: Many developers reach for getByTestId() or getByText() when getByRole() would be more appropriate. This creates:

• Tests that are tightly coupled to implementation details
• Tests that break when non-semantic changes are made
• Missed opportunities to verify accessibility

The Solution: Use getByRole() as your primary query method:

// ❌ Avoid: Implementation-specific queries
const button = getByTestId('submit-button');
const heading = container.querySelector('h1');

// ✅ Prefer: Semantic, accessible queries
const button = getByRole('button', { name: /submit/i });
const heading = getByRole('heading', { name: /welcome/i });

Why This Matters:

• getByRole() queries elements the way screen readers and users do
• Tests become more resilient to refactoring
• You’re implicitly testing accessibility
• Aligns with Testing Library’s philosophy: “The more your tests resemble the way your software is used, the more confidence they can give you”

When to Use Other Queries:

• getByLabelText() for form inputs
• getByPlaceholderText() only when label isn’t available
• getByTestId() as a last resort for elements that can’t be queried semantically

Avoid Unnecessary Curly Braces in JSX

The Problem: Overusing curly braces in JSX makes code harder to read and can lead to unnecessary re-renders:

// ❌ Unnecessary curlies
<div className={`header ${isActive ? 'active' : ''}`}>
  {children}
</div>

// ✅ Cleaner approach
<div className={classNames('header', { active: isActive })}>
  {children}
</div>

Best Practices:

• Use string literals when possible
• Use template literals for dynamic strings
• Leverage utility functions like classNames or clsx for conditional classes
• Only use curlies when you need JavaScript expressions

---

CSS and Styling Patterns

Add Classes in TSX Instead of Repeating CSS

The Problem: Repeating CSS rules across multiple components leads to:

• Code duplication
• Inconsistent styling
• Harder maintenance
• Larger bundle sizes

The Solution: Define reusable classes and apply them in your components:

// ❌ Bad: Repeated inline styles or duplicate CSS
<div style={{ marginTop: '1rem', padding: '1rem' }}>
  <div style={{ marginTop: '1rem', padding: '1rem' }}>
    {/* Nested repetition */}
  </div>
</div>

// ✅ Good: Reusable classes
<div className={styles.card}>
  <div className={styles.cardContent}>
    {/* Consistent styling */}
  </div>
</div>

Benefits:

• Single source of truth for styling
• Easier to maintain and update
• Better performance (CSS is cached)
• Consistent design system

Use Margin to Prevent Affecting Focus Rings

The Problem: When elements are positioned close together, padding can cause focus rings to expand unexpectedly

The Solution: Use margin instead of padding or positioning to create space around focusable elements:

<a class='find-us'>Find us</a>
<a>Support</a>

.find-us {
/*   ❌ Bad: padding-right: 30px; */
// ✅  Good:  
margin-right: 30px
}

.find-us:focus-visible {
  outline: 1px solid green;
 }

Key Points:

• Margin creates space outside the element’s box model
• Focus rings (outline) render outside the border
• Margin ensures the focus ring isn’t clipped by parent containers
• Use outline-offset for additional spacing if needed

When to Use Each:

• Margin: For spacing between elements, ensuring focus rings are visible
• Padding: For internal spacing within an element
• Gap: For spacing in flexbox/grid containers

Use Rem Instead of Pixel Values

The Problem: Hardcoded pixel values don’t respect user preferences and can cause accessibility issues:

/* ❌ Avoid: Fixed pixel values */
.element {
  font-size: 14px;
  padding: 3px;
  margin: 5px;
}

/* ✅ Prefer: Relative rem units */
.element {
  font-size: 0.875rem; /* 14px at base 16px */
  padding: 0.1875rem;   /* 3px at base 16px */
  margin: 0.3125rem;    /* 5px at base 16px */
}

Why Rem Matters:

• Respects user’s browser font size settings
• Better accessibility for users who need larger text
• More maintainable (change base font size, everything scales)
• Consistent scaling across the application

Conversion Guide:

• Base font size is typically 16px
• 1rem = 16px (by default)
• 0.75rem = 12px
• 0.875rem = 14px
• 1rem = 16px
• 1.25rem = 20px
• 1.5rem = 24px

Exceptions:

• Border widths (1px, 2px are fine)
• Box shadows (can use px)
• Very small decorative elements where exact pixel control matters

---

Prefer toHaveClass() Over toContain() for CSS Classes

The Problem: Using toContain() to check for CSS classes can lead to false positives and less precise tests:

// ❌ Problematic: Can match partial class names
expect(element.className).toContain('active');
// This would match: 'active', 'inactive', 'deactivate', etc.

// ❌ Also problematic: String matching on className
expect(element.className).toContain('btn-primary');
// Might match 'btn-primary-large' when you only want 'btn-primary'

The Solution: Use toHaveClass() which checks for exact class names:

// ✅ Better: Checks for exact class name
expect(element).toHaveClass('active');
expect(element).toHaveClass('btn-primary');

// ✅ Can check multiple classes
expect(element).toHaveClass('btn', 'btn-primary', 'btn-large');

// ✅ Can check for class absence
expect(element).not.toHaveClass('disabled');

Why toHaveClass() is Better:

• Precision: Only matches complete class names, not substrings
• Readability: More explicit about what you’re testing
• Safety: Prevents false positives from partial matches
• Multiple classes: Easily check for multiple classes at once
• Negation: Simple way to verify a class is NOT present

When toContain() Might Be Acceptable:

• Checking text content (not class names)
• Verifying substring matches in strings
• Testing error messages or dynamic content
• When you specifically need partial matching behavior

1. Always use toHaveClass() when checking CSS classes
2. Use toContain() only for text content or when partial matching is intentional
3. Check for class absence using .not.toHaveClass()
4. When checking multiple classes, list them all in one assertion for clarity
5. Consider using toHaveAttribute('class', ...) if you need to check the exact class string

Accessibility Considerations

Don’t Use aria-label on Elements Without Suitable Roles

The Problem: Adding aria-label to elements that don’t have semantic roles can confuse screen readers and create accessibility issues:

// ❌ Bad: aria-label on non-interactive element
<div aria-label="Click here to learn more">
  Some content
</div>

// ✅ Good: Use semantic elements or proper roles
<a href="/learn-more" aria-label="Learn more about our services">
  Learn more
</a>

// Or
<button aria-label="Close dialog">
  <Icon name="close" />
</button>

When aria-label is Appropriate:

• Interactive elements (buttons, links, form inputs)
• Elements with ARIA roles (role=”button”, role=”link”)
• Icon-only buttons where the icon needs a text alternative
• Form inputs where the label isn’t visible

When to Avoid aria-label:

• Plain <div> or <span> elements
• Non-interactive content
• When visible text already provides the label
• When aria-labelledby would be more appropriate

Best Practices:

1. Prefer semantic HTML elements
2. Use visible labels when possible
3. Use aria-labelledby to reference existing text
4. Only use aria-label when you can’t use visible text
5. Ensure the element has an appropriate role if it’s interactive

---

React and Component Patterns

Avoid Using Index in Key Values

The Problem: Using array indices as React keys can cause:

• Incorrect component state when list order changes
• Performance issues with re-renders
• Bugs with component state preservation
• Issues with animations and transitions

// ❌ Bad: Index as key
{items.map((item, index) => (
  <ListItem key={index} item={item} />
))}

// ✅ Good: Unique, stable identifier
{items.map((item) => (
  <ListItem key={item.id} item={item} />
))}

// ✅ Alternative: Generate stable key
{items.map((item) => (
  <ListItem key={`${item.type}-${item.id}`} item={item} />
))}

Why This Matters:

• React uses keys to identify which items have changed
• Index-based keys can cause state to be associated with the wrong item
• When items are reordered, React may reuse components incorrectly
• Can lead to subtle bugs that are hard to track down

Best Practices:

• Use unique IDs from your data
• Combine multiple fields if no single ID exists
• Generate stable keys that don’t change between renders
• Never use index for keys when list can be reordered, filtered, or items can be added/removed

When Index Might Be Acceptable:

• Static lists that never change order
• Lists where items are never added/removed
• Simple presentation components without internal state
• Even then, prefer a stable identifier if available

Destructure Objects Instead of Repeated Property Access

The Problem: Repeatedly accessing object properties makes code verbose and harder to read:

// ❌ Verbose: Repeated property access
function Component({ user, settings, theme }) {
  return (
    <div>
      <h1>{user.name}</h1>
      <p>{user.email}</p>
      <span>{user.role}</span>
      <div className={settings.layout}>
        <button style={{ color: theme.primary }}>
          {settings.buttonText}
        </button>
      </div>
    </div>
  );
}

// ✅ Clean: Destructured properties
function Component({ user, settings, theme }) {
  const { name, email, role } = user;
  const { layout, buttonText } = settings;
  const { primary } = theme;
  
  return (
    <div>
      <h1>{name}</h1>
      <p>{email}</p>
      <span>{role}</span>
      <div className={layout}>
        <button style={{ color: primary }}>
          {buttonText}
        </button>
      </div>
    </div>
  );
}

Benefits:

• More readable code
• Shorter, cleaner expressions
• Easier to see what properties are being used
• Can rename during destructuring if needed
• Better IDE autocomplete support

When to Destructure:

• When accessing the same object properties multiple times
• When you need to pass specific properties to child components
• When you want to make dependencies explicit
• When working with nested objects

When Not to Destructure:

• When you only access a property once
• When the object name provides important context
• When destructuring would make the code less clear

---

CSS Grid and Layout

Prevent Column Blowout with minmax()

The Problem: Using auto in grid columns can cause columns to grow unexpectedly, breaking layouts:

/* ❌ Problematic: Columns can grow too wide */
.grid {
  display: grid;
  grid-template-columns: auto 1fr auto;
}

/* If the first column has long content, it can push other columns off-screen */

The Solution: Use minmax() to constrain column sizes:

/* ✅ Better: Constrained column sizes */
.grid {
  display: grid;
  grid-template-columns: auto minmax(1rem, 1fr) auto;
}

/* Or more explicitly */
.grid {
  display: grid;
  grid-template-columns: 
    minmax(auto, max-content) 
    minmax(0, 1fr) 
    minmax(auto, max-content);
}

Understanding minmax():

• minmax(min, max) sets minimum and maximum sizes
• First value: minimum size the column can shrink to
• Second value: maximum size the column can grow to
• 1fr means “take available space”
• 0 or 1rem prevents columns from growing too large

Common Patterns:

/* Sidebar + Main content */
grid-template-columns: minmax(200px, 300px) 1fr;

/* Three columns with constraints */
grid-template-columns: 
  minmax(100px, auto) 
  minmax(0, 1fr) 
  minmax(100px, auto);

/* Responsive with minmax */
grid-template-columns: 
  minmax(0, 1fr) 
  minmax(250px, 400px) 
  minmax(0, 1fr);

Why This Matters:

• Prevents layout breaking with unexpected content
• Ensures responsive behavior
• Maintains design constraints
• Better control over grid behavior

Conclusion

Code reviews are learning opportunities for both reviewers and authors. These patterns represent common issues that, when addressed, lead to more maintainable, accessible, and performant code. I hope my experiences will be helpful to you in some cases