Review Pr

PR Review: #$0

Perform a comprehensive review of pull request #$0 in the fundamental-styles repository, checking against project coding standards, accessibility requirements, and architectural patterns.

Setup

If $0 is empty or not a number, ask the user for a PR number before proceeding.

Fetch PR Context

Gather all necessary information about the PR:

# PR metadata and description
gh pr view $0 --json title,body,author,state,isDraft,labels

# Full diff for review
gh pr diff $0

# List of changed files
gh pr diff $0 --name-only

Review Checklist

Review each changed file against the applicable sections below. Report findings grouped by severity:

• Blocking: Must fix before merge (breaks functionality, violates critical standards)
• Suggestion: Should fix (best practices, maintainability)
• Nit: Optional improvements (style preferences, minor optimizations)

1. Commit & PR Format

Verify commit messages and PR title follow the conventional format:

Expected format: <type>(<scope>): <subject>

Valid types:

• feat - New feature or component
• fix - Bug fix
• docs - Documentation changes
• style - CSS/SCSS styling changes (not code style)
• refactor - Code refactoring without behavior change
• test - Adding or updating tests
• build - Build system or dependency changes
• ci - CI/CD configuration changes
• chore - Maintenance tasks

Valid scopes:

• styles - SCSS/CSS changes
• docs - Documentation
• ci - CI/CD
• Component names (e.g., button, card, table)

Rules:

• [ ] PR title follows format
• [ ] Commit messages follow format
• [ ] No WIP prefix unless marked as draft
• [ ] Scope is specific and accurate

2. SCSS Code Style (for *.scss files)

Check against .claude/skills/scss-style-guide.md standards:

BEM Naming

• [ ] Block variable defined: $block: #{$fd-namespace}-component-name;
• [ ] Block classes: .#{$block}
• [ ] Element classes: .#{$block}__element
• [ ] Modifier classes: .#{$block}--modifier
• [ ] No deviations from strict BEM pattern

Self-Contained Components

• [ ] CRITICAL: No styling of other components (e.g., .fd-card .fd-button { })
• [ ] No cross-component dependencies
• [ ] Component works in isolation

Reset Mixin

• [ ] @include fd-reset(); present at start of every block
• [ ] @include fd-reset(); present for every element
• [ ] fd-reset() NOT used in modifiers (correct)

CSS Custom Properties Pattern

• [ ] CSS variables defined for all customizable properties
• [ ] Variable naming: --fd{Component}_{Property} format (e.g., --fdCard_Background)
• [ ] Properties use var(--fdComponent_Property) syntax
• [ ] Modifiers override variables, NOT properties directly
• [ ] Example:

  // ✅ CORRECT
  .#{$block} {
    --fdCard_Background: var(--sapTile_Background);
    background: var(--fdCard_Background);
    
    &--transparent {
      --fdCard_Background: transparent; // Override variable
    }
  }
  
  // ❌ WRONG
  .#{$block} {
    background: var(--sapTile_Background);
    
    &--transparent {
      background: transparent; // Setting property directly
    }
  }

Units & Values

• [ ] BLOCKING: No px units anywhere (use rem)
• [ ] Border widths in rem (e.g., 0.0625rem = 1px)
• [ ] Theming variables used when available (e.g., var(--sapTextColor))
• [ ] Unitless values for line-height, flex, z-index
• [ ] Zero doesn't need units (margin: 0;)

RTL Support - CRITICAL

• [ ] BLOCKING: No directional properties: margin-left, margin-right, padding-left, padding-right, left, right, border-left, border-right
• [ ] Logical properties used for horizontal spacing:
• margin-inline-start, margin-inline-end, margin-inline
• padding-inline-start, padding-inline-end, padding-inline
• inset-inline-start, inset-inline-end
• [ ] Logical properties used for borders:
• border-inline-start, border-inline-end
• border-start-start-radius, border-start-end-radius
• [ ] Vertical spacing uses explicit properties:
• margin-block-start, margin-block-end
• padding-block-start, padding-block-end
• [ ] RTL mixins used when logical properties insufficient:
• @include fd-set-position-right()
• @include fd-set-margin-left()
• [ ] Physical corner names avoided when direction matters

State & Interactive Patterns

• [ ] State mixins used: @include fd-hover(), @include fd-active(), @include fd-focus()
• [ ] State changes use CSS variable overrides
• [ ] Interactive elements have cursor: pointer
• [ ] Focus states have sufficient contrast
• [ ] Disabled states prevent interaction

File Structure

• [ ] Correct imports: @import "./new-settings";, @import "./mixins";
• [ ] Block variable defined before usage
• [ ] Logical organization: variables → mixins → block → elements → modifiers → states

Nesting Depth

• [ ] Maximum 3 levels of nesting
• [ ] No tag selectors (e.g., div, span)
• [ ] No universal selectors (*)
• [ ] No complex specificity chains

Loops & Maps

• [ ] Repetitive patterns use @each loops
• [ ] Size variations use maps
• [ ] Color variations use maps
• [ ] No duplicate code for similar variations

Responsive & Compact

• [ ] Responsive breakpoints use mixins: @include fd-media-sm(), @include fd-media-md()
• [ ] Compact mode uses: @include fd-compact-or-condensed()
• [ ] Compact mode overrides variables, not properties

3. Accessibility (for HTML examples and patterns)

Check against .claude/skills/accessibility-guide.md:

Semantic HTML

• [ ] Native HTML elements used when possible (prefer <button> over <div role="button">)
• [ ] Proper heading hierarchy (h1 → h2 → h3)
• [ ] Lists use <ul>, <ol>, or <dl>
• [ ] Tables use <table> with proper headers

Labels & Form Controls

• [ ] BLOCKING: All form inputs have associated labels
• [ ] for attributes match existing id values
• [ ] Labels are properly connected (not orphaned)
• [ ] Icon-only buttons have aria-label or aria-labelledby

ARIA Roles & Required Attributes

Check that roles have their required companion attributes:

• [ ] Range widgets (role="slider", "progressbar") have:
• aria-valuenow, aria-valuemin, aria-valuemax
• [ ] Checkable widgets (role="checkbox", "radio", "switch") have:
• aria-checked="true|false"
• [ ] Selectable widgets (role="option", "tab") have:
• aria-selected="true|false"
• [ ] Expandable widgets (role="combobox", disclosure buttons) have:
• aria-expanded="true|false"
• [ ] Popup triggers have:
• aria-haspopup="menu|listbox|dialog|true"
• [ ] Modal dialogs have:
• aria-modal="true"
• aria-labelledby or aria-label
• [ ] Tabs interface:
• tablist contains tab elements
• Each tab has aria-selected, aria-controls, unique id
• Each tabpanel has matching id and aria-labelledby
• [ ] Radio groups:
• radiogroup contains radio elements
• Only one radio has aria-checked="true"
• [ ] Menus:
• menu/menubar contains menuitem elements

ID References

• [ ] BLOCKING: All aria-labelledby values point to existing IDs
• [ ] BLOCKING: All aria-describedby values point to existing IDs
• [ ] BLOCKING: All aria-controls values point to existing IDs
• [ ] BLOCKING: All for attributes point to existing IDs
• [ ] IDs are unique within the page

Keyboard & Focus

• [ ] Interactive custom elements have tabindex="0" or tabindex="-1"
• [ ] No positive tabindex values (1, 2, 3, etc.)
• [ ] Focus indicators are visible
• [ ] Tab order is logical

Decorative vs Meaningful Content

• [ ] Decorative icons have aria-hidden="true"
• [ ] Meaningful icons have text alternatives
• [ ] Decorative images have empty alt (alt="")
• [ ] Meaningful images have descriptive alt text

4. Component Patterns

Component Structure

• [ ] Component follows BEM structure
• [ ] Component has main documentation page
• [ ] Examples are complete and functional
• [ ] No missing closing tags
• [ ] Proper HTML5 structure

Design Tokens

• [ ] Uses SAP theming variables (--sapTextColor, --sapTile_Background, etc.)
• [ ] No hardcoded colors (use tokens)
• [ ] No hardcoded font sizes (use tokens)
• [ ] Tokens used consistently

States & Modifiers

• [ ] State classes follow convention: is-error, is-disabled, is-selected, is-active
• [ ] States use ARIA attributes where applicable: aria-invalid, aria-disabled, aria-selected
• [ ] Disabled state uses both disabled attribute AND aria-disabled="true"

5. Documentation

• [ ] New components have README.md or documentation page
• [ ] CLAUDE.md updated if component patterns change
• [ ] Breaking changes documented in PR description
• [ ] Migration guide provided for breaking changes
• [ ] Examples are self-contained and runnable

6. Testing & Quality

• [ ] Visual regression tests exist or updated (if applicable)
• [ ] New components have test coverage
• [ ] Examples are tested in major browsers
• [ ] RTL mode tested (if layout changes)
• [ ] Compact mode tested (if size changes)
• [ ] No console errors in examples

7. File Organization

• [ ] Component files in correct directory structure
• [ ] Consistent file naming (kebab-case)
• [ ] No unnecessary files
• [ ] No commented-out code blocks
• [ ] No debug statements left in code

Common Anti-Patterns to Flag

Flag these as Blocking issues:

1. Cross-component styling: .fd-card .fd-button { } 2. Missing fd-reset(): Element without reset mixin 3. Using px units: Any px value in SCSS 4. Directional properties: margin-left, padding-right, left, right, border-left 5. Setting properties in modifiers: Instead of overriding variables 6. Missing ARIA labels: Icon-only buttons, form inputs 7. Invalid ID references: aria-labelledby pointing to non-existent ID 8. Missing required ARIA attributes: Slider without values, tab without aria-selected

Output Format

Provide findings as a structured summary:

## Review Summary: PR #1234

**Title:** [PR title]
**Author:** [author]
**Status:** [open/closed/merged]
**Overall Recommendation:** APPROVE / REQUEST CHANGES / COMMENT

---

### Blocking Issues (Must Fix)

#### [filename.scss:line]
- **Issue**: [Description of problem]
- **Fix**: [Specific fix with code example if applicable]

---

### Suggestions (Should Fix)

#### [filename.scss:line]
- **Issue**: [Description]
- **Improvement**: [Recommended approach]

---

### Nits (Optional)

#### [filename.scss:line]
- **Note**: [Minor improvement suggestion]

---

### Missing or Unclear

**Tests:**
- [ ] [List any new functionality that lacks tests]

**Documentation:**
- [ ] [List any API changes without docs]

**Breaking Changes:**
- [ ] [List any unannounced breaking changes]

---

### Positive Highlights

- [Mention good practices observed]
- [Call out particularly clean implementations]

---

### Next Steps

[Summarize what needs to happen before merge]

Review Approach

1. Start with structure: Check commit format, PR title, overall organization 2. File-by-file review: Go through each changed file systematically 3. Critical issues first: Flag blocking issues immediately 4. Context matters: Consider if this is a new component, refactor, or bug fix 5. Be specific: Include line numbers and exact fixes 6. Be constructive: Explain WHY something is an issue and HOW to fix it 7. Acknowledge good work: Call out positive implementations

Edge Cases

• Refactoring PRs: May have many changes; focus on consistency
• Bug fixes: Verify the fix doesn't introduce new issues
• New components: More thorough review of all patterns
• Documentation PRs: Focus on clarity, completeness, accuracy
• Dependency updates: Check for breaking changes in package.json

Additional Resources

Reference the project style guides:

• .claude/skills/scss-style-guide.md
• .claude/skills/accessibility-guide.md
• CLAUDE.md

---

Remember: The goal is to maintain code quality, accessibility, and consistency across fundamental-styles while being constructive and helpful to contributors.