Skip to content

Conversation

arjansingh
Copy link
Contributor

@arjansingh arjansingh commented Sep 15, 2025

Summary

Focus states weren't working, especially when trying to use the selects with TAB indexes and your keyboard.

Changes

  1. Added focus states and cleared them with design. See below.
  2. Added storybook stories
  3. Added aria labels since I was in there anyway.

Screenshots

Demo of working focus states:

MultiSelect-2025-09-15.mov
SingleSelect-2025-09-15.mov

┆Issue is synchronized with this Notion page by Unito

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Sep 15, 2025
Copy link

github-actions bot commented Sep 15, 2025

🎭 Playwright Test Results

All tests passed!

⏰ Completed at: 09/15/2025, 09:49:58 PM UTC

📈 Summary

  • Total Tests: 450
  • Passed: 421 ✅
  • Failed: 0
  • Flaky: 0
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 414 / ❌ 0 / ⚠️ 0 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

DrJKL
DrJKL previously approved these changes Sep 15, 2025
Copy link
Contributor

@DrJKL DrJKL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

selectedCount.value > 0
? 'border-blue-400 dark-theme:border-blue-500'
: 'border-transparent',
'focus-within:border-blue-400 dark-theme:focus-within:border-blue-500',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The border color is being set in two places. It makes sense that focus-within would take precedence.
Want to wrap this in a cn() to make sure the options merge in order?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it's a little weird that we'd have focus states on both the trigger and the menu item...

@arjansingh arjansingh force-pushed the feat/multiselect-singleselect-accessibility branch from 576d0e3 to eadbaf9 Compare September 15, 2025 21:39
@arjansingh arjansingh enabled auto-merge (squash) September 15, 2025 21:39
@arjansingh arjansingh merged commit 62897c6 into main Sep 15, 2025
20 checks passed
@arjansingh arjansingh deleted the feat/multiselect-singleselect-accessibility branch September 15, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants