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.

Sign up for GitHub

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

fix: add focused stylign to tabs #714

Merged
merged 1 commit into from
Nov 29, 2023
+7 −2
Merged

fix: add focused stylign to tabs #714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/components/src/core/Tabs/__snapshots__/index.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ exports[`<Tabs /> Default story renders snapshot 1`] = `
>
<button
aria-selected="true"
class="MuiButtonBase-root MuiTab-root MuiTab-textColorPrimary Mui-selected css-17rzz1x-MuiButtonBase-root-MuiTab-root"
class="MuiButtonBase-root MuiTab-root MuiTab-textColorPrimary Mui-selected css-1jnyyyh-MuiButtonBase-root-MuiTab-root"
role="tab"
tabindex="0"
type="button"
Expand All @@ -26,7 +26,7 @@ exports[`<Tabs /> Default story renders snapshot 1`] = `
</button>
<button
aria-selected="false"
class="MuiButtonBase-root MuiTab-root MuiTab-textColorPrimary css-17rzz1x-MuiButtonBase-root-MuiTab-root"
class="MuiButtonBase-root MuiTab-root MuiTab-textColorPrimary css-1jnyyyh-MuiButtonBase-root-MuiTab-root"
role="tab"
tabindex="-1"
type="button"
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/core/Tabs/style.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ export const StyledTab = styled(RawTab, {
color: black;
}
&.Mui-focusVisible {
outline: 5px auto Highlight;
outline: 5px auto -webkit-focus-ring-color;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is currently the same focus outline that buttons have:
Screenshot 2023-11-29 at 12 56 57 PM

I am not a designer, and I'm happy to change this to something else. Figured it made sense for now to keep it simple and match buttons and text inputs.

Note that mui has default focus styles which we're suppressing as part of the theme:

sci-components/packages/components/src/core/styles/common/defaultTheme.ts

Lines 283 to 298 in 9451847

MuiButtonBase: {
defaultProps: {
disableRipple: true,
},
},
MuiLink: {
defaultProps: {
underline: "hover",
},
},
MuiTab: {
defaultProps: {
disableRipple: true,
},
},
},

We could turn those back on, although it looks weird and would take tweaking colors for both buttons and tabs.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 hmm... Are we re-doing work that we can just get for free from Mui?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely are, although I think it would take design and eng work to get back to something that looks reasonable with the defaults for all components.

}
&.Mui-selected {
color: black;
Expand Down
Loading