Conversation
✅ Deploy Preview for phillips-seldon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR standardizes keyboard focus states across the component library by introducing a new focus-ring mixin that applies a consistent 2px blue outline with configurable parameters. It also improves the Accordion component's accessibility by changing its trigger from a div to a semantic button element, making it keyboard navigable.
Changes:
- Added
focus-ringSCSS mixin with configurable border-radius, outline-offset, outline-width, and outline-color parameters - Updated 17+ components and 3 patterns to use the standardized focus-ring mixin
- Changed Accordion trigger from div to button element with appropriate SCSS resets and updated tests
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scss/_utils.scss | Adds new focus-ring mixin for consistent focus styling |
| src/components/Accordion/_accordion.scss | Adds button style resets and focus-ring styling |
| src/components/Accordion/AccordionItem.tsx | Changes trigger from div to button element |
| src/components/Accordion/Accordion.test.tsx | Updates test expectations for locked accordion behavior |
| src/components/AddToCalendar/_addToCalendar.scss | Applies focus-ring mixin |
| src/components/Breadcrumb/_breadcrumb.scss | Applies focus-ring mixin |
| src/components/Button/_button.scss | Applies focus-ring mixin to all button variants |
| src/components/Carousel/_carousel.scss | Applies focus-ring mixin with custom width |
| src/components/ComboBox/_combobox.scss | Applies focus-ring mixin including error state |
| src/components/DatePicker/_datePicker.scss | Applies focus-ring mixin to calendar days |
| src/components/DescriptiveRadioButton/_descriptiveRadioButton.scss | Separates selected and focus states, applies focus-ring |
| src/components/Dropdown/_dropdown.scss | Applies focus-ring mixin |
| src/components/IconButton/_iconButton.scss | Applies focus-ring mixin and removes duplicate selector |
| src/components/Input/_input.scss | Applies focus-ring mixin (but error state needs refinement) |
| src/components/Link/_link.scss | Applies focus-ring mixin |
| src/components/Pagination/_pagination.scss | Applies focus-ring mixin |
| src/components/Search/_searchButton.scss | Adds focus-ring styling with zero offset |
| src/components/Search/Search.stories.tsx | Adjusts story container min-height |
| src/components/Select/_select.scss | Applies focus-ring mixin |
| src/components/Tabs/_tabs.scss | Updates box-shadow focus to use new focus color and width |
| src/components/Tags/_tags.scss | Applies focus-ring mixin |
| src/components/TextArea/_textArea.scss | Applies focus-ring mixin (but error state needs refinement) |
| src/patterns/FiltersInline/_filterButton.scss | Attempts to apply focus-ring but has parameter order bug |
| src/patterns/FavoritesCollectionTile/_favoritesCollectionTile.scss | Applies focus-ring mixin |
| src/patterns/CountryPicker/_countryPickerTrigger.scss | Applies focus-ring mixin |
| src/patterns/CountryPicker/_countryPickerOption.scss | Uses inset box-shadow approach for phone options |
| &:focus-visible { | ||
| border-radius: $radius-3xl; | ||
| @include focus-ring($border-radius: $radius-3xl); | ||
| } |
There was a problem hiding this comment.
Redundant focus-visible declaration. The tertiary variant already inherits the focus-ring styling from the combined selector on line 77-79 (which applies to both secondary and tertiary variants), making this duplicate declaration unnecessary. Consider removing this to reduce code duplication.
|
🚀 Storybook preview is ready. • Preview: https://68b9f094608b90f3cfec5a06-qirdrnjdkk.chromatic.com/ |
|
Sent to design for review, expect some tweaks |
Jira ticket
L3-7525
Screenshots
Figma link
Figma Link - Button Focus Example
Summary
Change List (describe the changes made to the files)
This pull request standardizes and improves focus styles across multiple components by replacing ad-hoc CSS with the shared
focus-ringmixin. It also includes an accessibility improvement for the Accordion component by switching its trigger from adivto abutton, and updates related tests and styles accordingly.Accessibility and Component Improvements:
divto abuttonfor better accessibility, updated associated test expectations, and reset button styles in_accordion.scssto maintain visual consistency. [1] [2] [3]Standardization of Focus Styles:
focus-ringmixin in the following components:Minor Updates:
These changes collectively improve accessibility, maintainability, and visual consistency across the component library.
Acceptance Test (how to verify the PR)
Regression Test
Evidence of testing
Things to look for during review
feat(scope): ...if aminorrelease should be triggered.phillipsclass prefix are using the prefix variabledata-testidattribute.New Components
index.tsfilecomponentStyles.scssfile.