[WEB-4036] Add Badge component, add focus-base utility class
#572
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces modifications to several components and styles in the codebase. Key changes include the introduction of a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/core/styles/utils.css (1)
1-3: Well-structured focus utility class that promotes accessibility!The
.focus-baseutility class effectively consolidates focus styles and follows accessibility best practices by:
- Using
focus-visiblefor better user experience- Providing clear visual indicators with 4px outline
- Maintaining consistent focus styling across components
Consider documenting this utility class in your component library's style guide to promote reuse and maintain consistency across teams.
src/core/Badge/Badge.stories.tsx (1)
12-53: Consider adding more story variations for comprehensive coverage.The current stories cover basic functionality well, but consider adding these variations:
- Color variants
- Size variations
- Combined states (e.g., hoverable + focusable)
- Error states
- Loading states
Here's a suggested addition:
export const WithCombinedStates = Template.bind({}); WithCombinedStates.args = { children: "BADGE", hoverable: true, focusable: true, iconBefore: "icon-gui-check-circled" }; export const WithError = Template.bind({}); WithError.args = { children: "ERROR", color: "error", iconBefore: "icon-gui-warning" };src/core/styles/buttons.css (2)
3-3: LGTM! Consider deprecating legacy button stylesThe modern button implementation with
focus-baseis clean and well-integrated. Consider gradually migrating components fromlegacy-buttons.cssto this newer implementation to maintain consistency.Migration strategy:
- Document the new button system as the preferred approach
- Mark legacy button classes as deprecated
- Create a migration guide for teams
- Set up deprecation warnings for legacy class usage
3-3: Consider documenting focus-base implementation guidelinesThe introduction of
focus-baseis a good step toward standardization, but its implementation varies across files. To ensure consistent adoption:
- Document the intended usage patterns of
focus-base- Specify whether it should be used alone or combined with other focus states
- Provide examples of correct implementation in different contexts (buttons, dropdowns, etc.)
Would you like help creating this documentation or setting up a style guide for focus management?
Also applies to: 3-3, 3-3
src/core/Badge.tsx (1)
69-91: Simplify the childClass computation logicThe current implementation has duplicate cases that can be simplified.
Consider this more concise implementation:
const childClass = useMemo(() => { - switch (size) { - case "xs": - case "sm": - return "leading-[18px]"; - case "md": - case "lg": - return "leading-[20px]"; - } + return ["xs", "sm"].includes(size) ? "leading-[18px]" : "leading-[20px]"; }, [size]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/core/Badge/__snapshots__/Badge.stories.tsx.snapis excluded by!**/*.snapsrc/core/Toggle/__snapshots__/Toggle.stories.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
src/core/Accordion.tsx(1 hunks)src/core/Badge.tsx(1 hunks)src/core/Badge/Badge.stories.tsx(1 hunks)src/core/Toggle.tsx(1 hunks)src/core/styles.components.css(1 hunks)src/core/styles/buttons.css(1 hunks)src/core/styles/dropdowns.css(1 hunks)src/core/styles/forms.css(2 hunks)src/core/styles/legacy-buttons.css(5 hunks)src/core/styles/text.css(1 hunks)src/core/styles/utils.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/styles.components.css
🔇 Additional comments (9)
src/core/Toggle.tsx (1)
16-17: Good adoption of focus-base utility while maintaining accessibility!
The switch component properly implements the new focus-base utility while preserving important features:
- Proper transitions
- Disabled states
- ARIA attributes (via Radix UI)
Let's verify that the focus styles are working correctly across all states:
✅ Verification successful
The previous search was too broad. Let's specifically look for Toggle-related test files and their contents to verify if there are proper accessibility tests in place, particularly for focus states.
We need to verify the focus-base utility implementation and the Toggle component's code to ensure the focus styles are properly implemented. The stories show basic functionality, but we need to see the actual implementation details.
Focus styles are properly implemented and consistent with design system
The Toggle component correctly implements the focus-base utility, which provides a standardized focus state across the design system:
- Uses Tailwind's focus-base utility that applies a 4px outline with proper offset and color
- Maintains accessibility through Radix UI's Switch component with proper ARIA attributes
- Focus styles are consistently applied alongside other interactive states (checked, disabled)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any test files related to Toggle component
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts Toggle
# Check if there are any existing accessibility tests
rg -l "accessibility|a11y|focus|keyboard" $(fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts Toggle)
Length of output: 1779
Script:
#!/bin/bash
# Let's check the Toggle component's test files and stories
fd Toggle.test -e tsx -e ts
fd Toggle.stories -e tsx -e ts
# Get the content of any Toggle test or story files to check for accessibility testing
fd Toggle -e test.tsx -e test.ts -e stories.tsx -e stories.ts -x cat {}
# Check for any accessibility-related tests in the Toggle component
rg -A 5 -B 5 "accessibility|a11y|focus|keyboard" src/core/Toggle
Length of output: 4368
Script:
#!/bin/bash
# Let's check the focus-base utility implementation
rg -A 5 -B 5 "focus-base" src/core/styles/
# Check the Toggle component implementation
cat src/core/Toggle.tsx
Length of output: 9335
src/core/Badge/Badge.stories.tsx (1)
20-20: Verify icon naming convention consistency.
The stories use icons with names like icon-gui-check-lotus-circled, icon-gui-chevron-down, and icon-gui-check-circled. Let's verify this naming pattern is consistent with the design system.
Also applies to: 26-26, 44-44, 51-51
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about icon naming patterns in the codebase.
Icon naming convention is consistent with the design system
The icon names used in Badge.stories.tsx follow the established pattern in the codebase. The icon-gui- prefix is consistently used across the codebase for GUI-related icons, and these icons are properly typed through the IconName type system defined in src/core/Icon/types.ts. The specific icons used:
icon-gui-check-lotus-circledicon-gui-chevron-downicon-gui-check-circled
are part of the GUI icon set (iconNames.gui) and follow the same naming convention used throughout the component library.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for icon usage patterns
rg "icon-gui-" -t tsx -t ts
# Check for icon definitions or configurations
fd -e tsx -e ts -e css | xargs rg "icon-gui-|iconName|IconName"
Length of output: 8580
src/core/styles/dropdowns.css (1)
3-3: Verify accessibility of new focus styling
While consolidating focus styles with focus-base improves consistency, we should verify that it maintains proper keyboard navigation visibility and meets WCAG requirements for focus indicators.
✅ Verification successful
Let me check the definition of focus-base to verify its accessibility properties.
Focus styling meets accessibility standards
The focus-base utility provides strong accessibility features through:
- Visible focus indicators using
focus-visible:outline-4 - High-contrast outline via
focus-visible:outline-gui-focus - Proper keyboard navigation support with
focus-visiblepseudo-class - Consistent implementation across buttons, inputs, and interactive elements
This consolidation maintains WCAG compliance while improving design consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for focus-base definition to verify it meets accessibility standards
rg -l "focus-base" "src/core/styles/"
# Check for any existing accessibility tests
fd -e test.js -e test.tsx -e spec.js -e spec.tsx | xargs rg -l "accessibility|focus|keyboard"
Length of output: 1896
Script:
#!/bin/bash
# Check the definition of focus-base in utils.css
rg "focus-base" src/core/styles/utils.css -A 5 -B 5
# Look for any accessibility-related comments or documentation
rg "@apply.*focus" src/core/styles/ -A 2 -B 2
Length of output: 6523
src/core/Badge.tsx (2)
1-55: LGTM! Well-structured props interface with comprehensive documentation.
The BadgeProps interface is well-designed with:
- Clear JSDoc documentation for each prop
- Appropriate type constraints
- Good default values in the implementation
93-112: LGTM! Comprehensive color mapping with dark mode support.
The color class computation is well-implemented with:
- Consistent color mapping
- Dark mode consideration for neutral color
- Appropriate performance optimization
src/core/styles/forms.css (1)
126-126: LGTM! Consistent implementation of focus-base utility
The changes successfully standardize focus styles across form elements:
- ui-input: Replaced individual focus styles with focus-base
- ui-radio: Unified focus handling with other components
Also applies to: 143-143
src/core/styles/text.css (1)
179-180: LGTM! Standardized focus styles for links
The changes successfully implement the focus-base utility while maintaining the existing link behaviors:
- ui-link: Preserves default text color during focus
- ui-link-neutral: Maintains charcoal-grey text color during focus
Also applies to: 185-186
src/core/Accordion.tsx (2)
100-102: LGTM: Focus styling maintains accessibility
The conditional application of focus-base for non-static themes while preserving focus-visible:outline-none for static themes maintains proper keyboard navigation accessibility. This change aligns well with the PR objective of standardizing focus management across components.
101-101: Verify the focus-base utility class implementation
The change to use the new focus-base utility class looks good, but let's verify its implementation to ensure it provides equivalent styling to the replaced focus classes.
✅ Verification successful
Focus-base utility class is properly implemented and widely adopted
The focus-base utility class is well-defined in utils.css with proper focus management styles using outline properties. It's already being used consistently across multiple components (Badge, Toggle, Accordion) and applies standardized focus styling through Tailwind's @apply directive. The implementation in Accordion maintains accessibility by applying focus styles only for interactive themes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the focus-base utility class exists and check its implementation
# Expected: Should find the utility class definition with equivalent focus styles
# Search for the focus-base utility class definition
echo "Searching for focus-base utility class definition..."
rg -t css "focus-base"
# Search for any other components using the new utility class
echo "Checking adoption of focus-base across components..."
rg "focus-base" --type tsx
Length of output: 1466
Script:
#!/bin/bash
# Let's try searching for .tsx files using ripgrep's file extension pattern
echo "Checking adoption of focus-base across components..."
rg "focus-base" -g "*.tsx"
# Let's also check the implementation of focus-base utility class
echo "Checking focus-base utility class implementation..."
rg -A 5 "\.focus-base" src/core/styles/utils.css
Length of output: 948
jamiehenson
force-pushed
the
badge-component
branch
from
efbd0a7 to
c6a039c
Compare
jamiehenson
force-pushed
the
badge-component
branch
from
c6a039c to
fdd4d8b
Compare
There was a problem hiding this comment.
LGTM, will defer final ✅ to @aralovelace because this directly affects her work :D
jamiehenson
force-pushed
the
badge-component
branch
from
fdd4d8b to
bd53ad3
Compare
Jira Ticket Link / Motivation
@aralovelace has used a temporary local badge for the latest Pricing calculator design. This PR adds a Badge component to
ably-uito support this, and other badge-oriented requirements in future.Summary of changes
Badgecomponentfocus-baseclass which replaces all of the repeated focus/focus-visible classes around the codeHow do you manually test this?
📖 https://ably.github.io/ably-ui/?path=/docs/components-badge--docs
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Release Notes
New Features
Badgecomponent for displaying customizable badges.Badgecomponent showcasing various states and styles.Improvements
.focus-baseclass.Styles
These changes enhance user experience and accessibility across the application.