Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[joy][Select] Set focus visible on select options when navigating with arrow keys #36689

Merged
merged 3 commits into from
Apr 23, 2023
Merged

[joy][Select] Set focus visible on select options when navigating with arrow keys #36689

merged 3 commits into from
Apr 23, 2023

Conversation

gitstart
Copy link
Contributor

@gitstart gitstart commented Mar 30, 2023

Description

Sets focus visible on <Li> when navigating with arrowkeys. @mui/joyui

Closes #36601

select-background.mov

This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.

Co-authored-by: raj360 <34602861+raj360@users.noreply.github.com>
@mui-bot
Copy link

mui-bot commented Mar 30, 2023

Netlify deploy preview

https://deploy-preview-36689--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 71b1423

@gitstart gitstart marked this pull request as ready for review March 30, 2023 07:41
@gitstart
Copy link
Contributor Author

@ZeeshanTamboli this PR is ready for review

@zannager zannager added component: select This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Mar 30, 2023
@ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli this PR is ready for review

I'll let @siriwatknp review this PR.

@ZeeshanTamboli ZeeshanTamboli changed the title [joy][Select] set focus visible on select options when navigating with arrow keys [joy][Select] Set focus visible on select options when navigating with arrow keys Mar 31, 2023
@gitstart
Copy link
Contributor Author

What's the verdict on this? @siriwatknp

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

The usage of the token should not change. Let's change the token value instead:

diff --git a/packages/mui-joy/src/styles/extendTheme.ts b/packages/mui-joy/src/styles/extendTheme.ts
index 5a2513dcc3..cc8d6f3738 100644
--- a/packages/mui-joy/src/styles/extendTheme.ts
+++ b/packages/mui-joy/src/styles/extendTheme.ts
@@ -356,7 +356,7 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme {
       background: {
         body: getCssVarColor('palette-neutral-900'),
         surface: getCssVarColor('palette-common-black'),
-        popup: getCssVarColor('palette-neutral-800'),
+        popup: getCssVarColor('palette-neutral-900'),
         level1: getCssVarColor('palette-neutral-800'),
         level2: getCssVarColor('palette-neutral-700'),
         level3: getCssVarColor('palette-neutral-600'),

@gitstart
Copy link
Contributor Author

The usage of the token should not change. Let's change the token value instead:

diff --git a/packages/mui-joy/src/styles/extendTheme.ts b/packages/mui-joy/src/styles/extendTheme.ts
index 5a2513dcc3..cc8d6f3738 100644
--- a/packages/mui-joy/src/styles/extendTheme.ts
+++ b/packages/mui-joy/src/styles/extendTheme.ts
@@ -356,7 +356,7 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme {
       background: {
         body: getCssVarColor('palette-neutral-900'),
         surface: getCssVarColor('palette-common-black'),
-        popup: getCssVarColor('palette-neutral-800'),
+        popup: getCssVarColor('palette-neutral-900'),
         level1: getCssVarColor('palette-neutral-800'),
         level2: getCssVarColor('palette-neutral-700'),
         level3: getCssVarColor('palette-neutral-600'),

@siriwatknp thanks for the feedback, your comment has been addressed.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for the fix!

@siriwatknp siriwatknp merged commit d23221a into mui:master Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Select] Highlighted option not visible in dark mode
5 participants