[WEB-4122] Fix component focus behaviour, add theming to Accordion #566
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 includes updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (4)
src/core/FeaturedLink.tsx (1)
Line range hint
32-35: Fix typo in rel attribute valueThere's a typo in the
relattribute value: "noopenner" should be "noopener".Apply this fix:
- props.rel = "noopenner noreferrer"; + props.rel = "noopener noreferrer";src/core/styles/buttons.css (1)
3-3: LGTM! Consider adding a comment explaining the focus-visible patternThe change correctly implements the focus-visible pattern, showing focus indicators only during keyboard navigation while hiding them during mouse interactions. This improves both accessibility and visual aesthetics.
Consider adding a comment explaining this pattern for future maintainers:
+ /* Show focus outline only during keyboard navigation */ @apply inline-flex rounded justify-center items-center gap-12 text-neutral-000 transition-colors focus:outline-none focus-visible:outline-offset-0 focus-visible:outline-4 focus-visible:outline-gui-blue-focus disabled:cursor-not-allowed;src/core/styles/forms.css (1)
126-126: Consider consolidating outline propertiesWhile the implementation is correct, the outline properties could be consolidated for better maintainability.
- @apply hover:border-neutral-700 focus:bg-white focus:outline-none focus-visible:outline-gui-focus focus-visible:outline-offset-0 focus-visible:outline-4; + @apply hover:border-neutral-700 focus:bg-white focus:outline-none focus-visible:[outline:4px_solid_theme(colors.gui-focus)_offset-0];src/core/Accordion/Accordion.stories.tsx (1)
Line range hint
1-262: Consider documenting color token usageThe story implementations now use semantic color tokens (neutral-1300, neutral-000) consistently. Consider adding a comment or documentation section explaining the color token strategy for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
package.json(1 hunks)src/core/Accordion.tsx(1 hunks)src/core/Accordion/Accordion.stories.tsx(3 hunks)src/core/Accordion/types.ts(3 hunks)src/core/Accordion/utils.ts(1 hunks)src/core/FeaturedLink.tsx(4 hunks)src/core/MeganavItemsMobile.tsx(1 hunks)src/core/TabMenu.tsx(1 hunks)src/core/Toggle.tsx(1 hunks)src/core/styles/buttons.css(1 hunks)src/core/styles/dropdowns.css(1 hunks)src/core/styles/forms.css(3 hunks)src/core/styles/legacy-buttons.css(5 hunks)src/core/styles/text.css(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Learnings (1)
src/core/Accordion/types.ts (1)
Learnt from: jamiehenson
PR: ably/ably-ui#505
File: src/core/Accordion.tsx:64-79
Timestamp: 2024-11-12T07:31:53.691Z
Learning: In `src/core/Accordion.tsx`, for the `static` and `darkStatic` themes of the `Accordion` component, it's intentional that `toggleIconColor` matches the background color, making the toggle icon invisible.
🔇 Additional comments (18)
src/core/Toggle.tsx (1)
16-16: LGTM! Focus behavior changes align with accessibility requirements.
The changes correctly implement the focus visibility pattern:
- Removes default focus ring with
focus:outline-none - Shows focus outline only during keyboard navigation with
focus-visible - Maintains consistent styling with other components
Let's verify the keyboard navigation behavior:
✅ Verification successful
Focus behavior changes are consistent with accessibility best practices
The codebase shows consistent implementation of focus visibility patterns across components:
- All interactive elements properly remove default focus with
focus:outline-noneand add visible focus indicators withfocus-visible - Focus indicators use consistent styling:
- 4px outline width (
focus-visible:outline-4) - 0px offset (
focus-visible:outline-offset-0) - Standard color via
focus-visible:outline-gui-focus
- 4px outline width (
- Pattern is applied uniformly in buttons, links, form controls and other interactive elements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent focus-visible implementation across components
# Look for any components that might be missing the focus-visible pattern
# Search for potential inconsistencies in focus handling
rg -l 'focus:outline(?!-none)' --type tsx --type css
# Check for components using focus without focus-visible
rg -l 'focus:outline-none(?!.*focus-visible)' --type tsx --type css
Length of output: 622
Script:
#!/bin/bash
# Search for components with focus outline styles
rg --pcre2 'focus:outline(?!-none)' --type tsx --type css
# Find components that remove focus outline without focus-visible
rg --pcre2 'focus:outline-none(?!.*focus-visible)' --type tsx --type css
# Get a broader view of focus-related styles
rg 'focus[:-]' --type tsx --type css -A 2
Length of output: 262
Script:
#!/bin/bash
# Search for components with focus outline styles
rg 'focus:outline' -A 2
# Get a broader view of focus-related styles
rg 'focus[:-]' -A 2
# Find all TypeScript/React files that might have focus-related styles
fd '\.(tsx|ts|jsx|js)$' -x cat {} | rg 'focus' -A 2
Length of output: 68988
src/core/Accordion/utils.ts (4)
13-18: LGTM! Transparent theme properly handles dark mode.
The transparent theme implementation correctly maintains transparency while adding dark mode text colors.
21-26: LGTM! Static theme maintains consistent behavior in dark mode.
The static theme properly implements dark mode variants while maintaining its core behavior.
30-33: LGTM! Theme check functions simplified.
The theme checking functions have been simplified while maintaining their core functionality.
4-10: Verify the impact of theme consolidation on existing implementations.
The new default theme implementation looks good with proper dark mode support, but this is a breaking change that needs verification.
Let's check for existing usage:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Theme changes are safe to proceed with
The theme consolidation changes are non-breaking as:
- The
accordionThemestype still includes all previously supported themes: "default", "transparent", and "static" - No removed themes ("darkTransparent" or "darkStatic") were found in the codebase
- Dark mode support is added through Tailwind's dark variant (
dark:) without changing the theme API - Stories demonstrate proper theme usage and transitions between light/dark modes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for impacts of theme changes
# Look for existing usage of removed themes and verify migration needs
# Search for usage of old theme names
rg -A 2 'darkTransparent|darkStatic' --type tsx
# Check for direct theme string usage
rg -A 2 '"dark"|"light"' --type tsx
Length of output: 143
Script:
#!/bin/bash
# Let's try different approaches to find theme usage
# Search for Accordion imports and usage
rg -l "from.*Accordion"
# Search for theme-related patterns in all files
rg -A 2 'darkTransparent|darkStatic'
# Look for theme prop usage in components
rg -A 2 'theme=|theme:'
# Search specifically in TypeScript/JavaScript files for dark/light themes
rg -A 2 '"dark"|"light"' --type ts --type js
# Check for any direct references to the theme object
rg -A 2 'AccordionTheme'
Length of output: 7351
Script:
#!/bin/bash
# Let's check the types and theme definitions to understand the breaking changes
# Check the types file for theme definitions
cat src/core/Accordion/types.ts
# Check the story file to understand theme usage examples
cat src/core/Accordion/Accordion.stories.tsx
# Look for any theme-related tests
rg -l "test|spec" | xargs rg -l "Accordion.*theme"
Length of output: 10241
src/core/styles/dropdowns.css (1)
3-3: LGTM! Focus behavior aligned with component standards.
The dropdown focus behavior now correctly follows the project's focus visibility pattern, matching other components.
Let's verify consistency across dropdown variants:
✅ Verification successful
Focus behavior is consistently implemented across dropdown variants
The search results confirm that all dropdown variants inherit their focus behavior from the .ui-dropdown-base class, which contains the new focus-visible pattern. This is evident from:
- The main
.ui-dropdownclass extends.ui-dropdown-base - The
.ui-dropdown-smallvariant extends.ui-dropdown-base - The Select2 wrapper components also use these base classes
- No other conflicting focus styles were found in dropdown-related code
The implementation maintains consistency across all dropdown variants while properly implementing the new focus visibility pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent focus handling across dropdown variants
# Ensure all dropdown-related classes follow the same pattern
# Search for other dropdown-related classes that might need updates
rg -A 2 'dropdown.*focus' --type css
# Check for any remaining direct focus styles in dropdown-related code
rg -A 2 'focus(?!-visible).*dropdown' --type css
Length of output: 325
Script:
#!/bin/bash
# Let's try a different approach to check dropdown-related focus styles
# Search for focus-related styles in CSS files
rg --type css -A 3 'dropdown'
# Also search for any other selectors that might contain dropdown-related focus styles
rg --type css -A 3 '\.[a-zA-Z-]*dropdown[a-zA-Z-]*\s*{'
Length of output: 4288
src/core/FeaturedLink.tsx (1)
58-68: Great implementation of keyboard focus states!
The use of focus-visible pseudo-class properly implements the desired focus behavior - showing focus rings only during keyboard navigation while hiding them during mouse clicks. The implementation follows accessibility best practices.
src/core/Accordion/types.ts (1)
42-42: Well-structured theme system improvements
The migration from ColorClass to ColorThemeSet and the simplification of theme options create a more maintainable and flexible theming system. The changes align well with the PR objectives for dynamic theming.
Note: Based on previous learnings, the static theme intentionally matches toggleIconColor with the background color.
Let's verify that all theme usages have been updated:
Also applies to: 53-78
✅ Verification successful
Theme system changes have been properly implemented
The verification shows that:
- All theme usages in Accordion components are using the new theme system
- The stories file correctly imports and uses the new
accordionThemes - No references to old themes (
darkTransparent,lightTransparent) were found - Theme props and defaults are properly typed and aligned with the new theme options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining ColorClass usages in Accordion-related files
rg "ColorClass.*Accordion"
# Search for old theme names that should have been migrated
rg -l "darkTransparent.*Accordion|lightTransparent.*Accordion"
Length of output: 93
Script:
#!/bin/bash
# Let's broaden the search to find any theme-related usages in Accordion files
rg -A 2 "theme[\"'].*(?i:accordion)"
# Search for any references to the old themes (darkTransparent, lightTransparent) without specifically tying to Accordion
rg "darkTransparent|lightTransparent"
# Find all files related to Accordion to check theme usage
fd -e ts -e tsx "accordion" -i
# Check theme-related imports and usages in Accordion files
fd -e ts -e tsx "accordion" -i --exec rg -l "theme|Theme"
Length of output: 301
Script:
#!/bin/bash
# Let's check the actual theme usage in these files
rg -A 5 "theme.*=|theme:.*" src/core/Accordion.tsx src/core/Accordion/Accordion.stories.tsx
# Check if there are any imports of the theme types
rg "import.*theme|from.*theme" -i src/core/Accordion.tsx src/core/Accordion/Accordion.stories.tsx
# Look for any theme-related props or interfaces
ast-grep --pattern 'interface $_ {
$$$
theme$_
$$$
}'
Length of output: 3136
src/core/styles/legacy-buttons.css (1)
6-6: Excellent focus style implementation across button variants
The focus styles have been consistently implemented across all button variants:
- Uses
focus-visiblefor keyboard-only focus indication - Maintains consistent outline width (4px)
- Preserves color contrast for accessibility
- Retains existing hover/active/disabled states
This implementation aligns perfectly with the PR objectives for proper focus behavior.
Also applies to: 28-28, 46-46, 56-56, 66-66
src/core/styles/forms.css (2)
74-74: LGTM! Focus management for textarea is well implemented
The textarea focus management aligns with the PR objectives and maintains a good balance between aesthetics and accessibility.
143-143: LGTM! Radio button focus styling is well implemented
The radio button focus management correctly implements the focus-visible pattern while maintaining the component's visual hierarchy.
src/core/TabMenu.tsx (1)
138-138: LGTM! Tab focus management is well implemented
The focus management in the Tabs.Trigger component:
- Correctly implements the focus-visible pattern
- Maintains compatibility with dark mode
- Preserves existing interactive states (hover, active, disabled)
- Works well with the component's flexible width and height options
src/core/styles/text.css (1)
179-179: LGTM! Focus style enhancement improves accessibility
The change to use focus-visible pseudo-class is a good improvement as it shows the focus ring only during keyboard navigation while hiding it during mouse clicks, following modern accessibility best practices.
src/core/Accordion.tsx (1)
95-102: LGTM! Enhanced focus handling with proper theme support
The changes improve accessibility by:
- Removing default focus outline and using focus-visible
- Properly handling focus states based on theme
- Using appropriate outline width (4px) for better visibility
The implementation correctly differentiates between static and non-static themes.
src/core/MeganavItemsMobile.tsx (1)
88-88: LGTM! Consistent focus handling for search clear button
The change aligns with the project-wide focus style improvements, ensuring consistent behavior across components and better accessibility for keyboard navigation.
src/core/Accordion/Accordion.stories.tsx (3)
9-11: LGTM: Improved text contrast with theme support
The updated color classes provide better contrast ratios and proper dark mode support using Tailwind's conventional approach.
212-212: Verify complete removal of darkStatic theme references
The documentation has been updated to remove the darkStatic theme reference. Let's ensure all other references have been removed as well.
✅ Verification successful
No remaining references to darkStatic theme found
The search for "darkStatic" theme references across all TypeScript and TSX files returned no results, confirming that all references have been properly removed from the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to darkStatic theme
rg -i "darkstatic|dark.*static|static.*dark" --type typescript --type tsx
Length of output: 110
Script:
#!/bin/bash
# Search for any remaining references to darkStatic theme with correct file extensions
rg -i "darkstatic|dark.*static|static.*dark" -g "*.ts" -g "*.tsx"
Length of output: 67
58-62: Verify the impact of removed background styles
The div's className has been simplified, removing specific background styling. While this aligns with the theming improvements, please verify that the accordion themes are still visually distinct in the Storybook preview.
✅ Verification successful
Let me check the actual content of these files and the Accordion component to better understand the styling changes.
Let me check the Accordion component's theme-related code to understand how the styling is being handled.
Based on the search results, I can now generate the final response as I have sufficient information about the accordion theming implementation.
The background styling removal is intentional and safe
The removed background styling from the div is correct as the accordion themes are handled through a dedicated theming system defined in utils.ts. The themes are properly implemented with both light and dark mode variants through the themeClasses object, which provides background colors (bg), hover states (hoverBg), and text colors for each theme variant. The visual distinction between themes is maintained through these theme-specific styles rather than through the story wrapper div.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any background-related styles that might have been moved to a different location
rg -l "bg-.*neutral" --type css
Length of output: 177
Script:
#!/bin/bash
# Check the content of the Accordion component and related files
rg -A 5 "bg-.*neutral" src/core/styles/*.css
rg -A 5 "AccordionPresentation" src/core/Accordion/
Length of output: 11046
Script:
#!/bin/bash
# Check the Accordion component's theme implementation
rg -A 10 "theme" src/core/Accordion/
ast-grep --pattern 'className={$_}' src/core/Accordion/Accordion.tsx
Length of output: 5634
jamiehenson
force-pushed
the
component-focus-fix
branch
2 times, most recently
from
10e5508 to
08f023f
Compare
jamiehenson
force-pushed
the
component-focus-fix
branch
from
08f023f to
09610c9
Compare
| } ${additionalCSS}`} | ||
| className={cn( | ||
| "font-sans font-bold block group/featured-link", | ||
| { "text-gui-unavailable pointer-events-none": disabled }, |
There was a problem hiding this comment.
[question] would the convention not be to use one object here rather than 3?
There was a problem hiding this comment.
clsx is pretty open ended - we could do this with the "class as key, condition as value" object convention as is here, or have this as a ternary, or whatever really. I went with the former here to cut the line length down. You could have all of this as one object, and have the always-on classes with true as the value, but that seems verbose to me here.
I like it's flexibility though. If I were to make a fix here it would be to remove the object wrapping for l67
edit: made that change - it outputted "additionalCSS" into the class names which is obvs not what we want
jamiehenson
force-pushed
the
component-focus-fix
branch
from
09610c9 to
f0cf003
Compare
There was a problem hiding this comment.
I'm happy, do hold for @aralovelace's ✅ as well if you feel it is warranted :D
Jira Ticket Link / Motivation
Motivated by conversations with Jamie W about theming focusable components properly - we should be able to see a focus state when navigating by keyboard, but not a direct click. After studying
shadcnas a reference, I figured that thefocus-visiblepsuedo-class is the way to go.Summary of changes
Adds
focus-visibleclasses to focusable components and hides defaultfocusoutlines.As an extra, makes the
Accordioncomponent dynamically themable instead of relying on defined light and dark themes. This will require a very small correction in Voltaire as there's one place that references thedarkTransparenttheme - which will no longer exist.How do you manually test this?
Book
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
@ably/uiproject to 15.1.1.Accessibility Improvements
Accordion,FeaturedLink,MeganavItemsMobile,TabMenu, andToggle, ensuring outlines are shown only when focused via keyboard navigation.Styling Updates
Documentation
StaticAndFullyOpenstory in theAccordioncomponent.