-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: Default color mode to user's current system theme #3456
Conversation
Bundle ReportChanges will increase total bundle size by 6.09kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will increase total bundle size by 6.09kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #3456 +/- ##
==========================================
- Coverage 99.15% 99.10% -0.05%
==========================================
Files 809 806 -3
Lines 14307 14114 -193
Branches 3960 3958 -2
==========================================
- Hits 14186 13988 -198
- Misses 112 117 +5
Partials 9 9
... and 28 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3456 +/- ##
==========================================
- Coverage 99.15% 99.10% -0.05%
==========================================
Files 809 806 -3
Lines 14307 14114 -193
Branches 3967 3958 -9
==========================================
- Hits 14186 13988 -198
- Misses 112 117 +5
Partials 9 9
... and 28 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #3456 +/- ##
==========================================
- Coverage 99.15% 99.10% -0.05%
==========================================
Files 809 806 -3
Lines 14307 14114 -193
Branches 3960 3958 -2
==========================================
- Hits 14186 13988 -198
- Misses 112 117 +5
Partials 9 9
... and 28 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
@@ -261,7 +263,7 @@ | |||
@apply darkMode; | |||
} | |||
|
|||
/* new body theme can be applied here | |||
/* new body theme can be applied here */ | |||
|
|||
@media (prefers-color-scheme: dark) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is uncommented now
@@ -29,8 +29,15 @@ interface ThemeContextProviderProps { | |||
export const ThemeContextProvider: FC<ThemeContextProviderProps> = ({ | |||
children, | |||
}) => { | |||
const currentTheme = (localStorage.getItem('theme') as Theme) || Theme.LIGHT | |||
const [theme, setTheme] = useState<Theme>(currentTheme) | |||
const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only logic changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on moving this logic to a <script>
in the <head>
of the index.html
? That should mean the user doesn't have to wait for the JS to be downloaded, and executed to have the correct colour set, when they visit the site 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider that approach, though I ended up on preferring to have all the theme logic in one place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of mocking this out in all of these tests, can you mock it out in the vitest.config.mjs
similar to how we're globally mocking out some Sentry stuff?
This reverts commit 3ac0afa.
For sure, updated in 5a47a97 |
Description
This PR aims to add logic to default the user's appearance mode to whatever their current system theme is at the time they come to Codecov.
Originally I wanted to have the user's system theme be swappable, but I was running into some funky behavior with the instances where we had the "dark:" class name not being picked up correctly. In the end, I feel like that'd be a cute little easter egg at best, and just having the default system theme setup for when the user first comes onto the site is probably a nice Quality of Life adjustment anyway.
Worst case they just push the light/dark mode button 😄
Closes codecov/engineering-team#2403
Notable Changes
Screenshots
Screen.Recording.2024-10-30.at.3.02.58.PM.mov
Link to Sample Entry
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.