Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive dark mode theme system for the blog, introducing a NestedThemeSwitcher component, centralizing theme initialization and synchronization scripts, and updating layouts, components, and SCSS styles to support dark mode. Feedback on the changes highlights a style guide violation where unawaited calls in ThemeSwitcher.dispose should include explanatory comments.
| unawaited(_pageShowSubscription?.cancel()); | ||
| unawaited(_storageSubscription?.cancel()); | ||
| unawaited(_mediaQuerySubscription?.cancel()); |
There was a problem hiding this comment.
According to the repository style guide, any futures that are deliberately detached with unawaited must include a comment explaining why. Please add a brief comment explaining why these subscriptions are cancelled asynchronously.
| unawaited(_pageShowSubscription?.cancel()); | |
| unawaited(_storageSubscription?.cancel()); | |
| unawaited(_mediaQuerySubscription?.cancel()); | |
| // Cancel subscriptions asynchronously on dispose. | |
| unawaited(_pageShowSubscription?.cancel()); | |
| unawaited(_storageSubscription?.cancel()); | |
| unawaited(_mediaQuerySubscription?.cancel()); |
References
- Futures should be awaited, returned, or deliberately detached with
unawaitedand a comment explaining why. (link)
|
Staged preview of the updated docs.flutter.dev site (updated for commit 5709fd4): https://flutter-docs-prod--docs-pr13954-blog-dark-mode-97n6shd9.web.app |
|
Staged preview of the updated flutter.dev site (updated for commit 5709fd4): https://flutter-dev-230821--www-pr13954-blog-dark-mode-tcpu8yil.web.app |
parlough
left a comment
There was a problem hiding this comment.
Thanks for exploring this @conooi!
Generally looks great and like a good direction, with most of my remaining concerns about generalizing the style updates so future updates and maintenance are easier. Perhaps the capability can be expanded in the future as well.
Let me know if you have any questions or if you'd prefer I tackle any of the suggestions. Thanks again :D
| final oppositeId = isDark ? _Theme.light.id : _Theme.dark.id; | ||
|
|
||
| for (final element in [ | ||
| web.document.documentElement, |
There was a problem hiding this comment.
Here and elsewhere that documentElement is used (which I believe will resolve to the html element, can we instead just use web.document.body? That way we match the docs sites and there's a singular consistent location where the theme is configured. Then the styles can be simplified as well.
| letter-spacing: normal; | ||
| } | ||
|
|
||
| #theme-switcher { |
There was a problem hiding this comment.
Consider extracting this out to another dedicated file in the case we bring this to other parts of the site. These styles don't require the blog page.
| window.addEventListener('pageshow', applyStoredTheme); | ||
| window.addEventListener('storage', applyStoredTheme); | ||
| window | ||
| .matchMedia('(prefers-color-scheme: dark)') | ||
| .addEventListener('change', applyStoredTheme); |
There was a problem hiding this comment.
We don't need to listen to these events here since the Dart code already does. This script should be kept as focused initialization code like it was before. Similar to:
try {
const storedTheme = window.localStorage.getItem('theme') ?? 'light-mode';
const isAuto = storedTheme === 'auto-mode';
const isDark = isAuto
? window.matchMedia('(prefers-color-scheme: dark)').matches
: storedTheme === 'dark-mode';
document.body.classList.remove('light-mode', 'dark-mode', 'auto-mode');
document.body.classList.add(isDark ? 'dark-mode' : 'light-mode');
if (isAuto) document.body.classList.add('auto-mode');
} catch (_) {
// localStorage is not available; fall back to default light theme.
}Then we don't need the later themeSyncBodyScript either.
| .text( | ||
| 'html.dark-mode, ' | ||
| 'html.dark-mode body.blog, ' | ||
| 'html.dark-mode body.blog main { ' | ||
| 'background-color: #121317; color: #dcdcdc; color-scheme: dark; ' | ||
| '} ' | ||
| 'html.dark-mode body.blog ' | ||
| 'header.site-header:not(.mobile-nav-open), ' | ||
| 'html.dark-mode body.blog .site-footer { ' | ||
| 'background-color: #1c1e27; ' | ||
| '}', |
There was a problem hiding this comment.
I'm not sure what this is for. If these styles are needed, I believe they can be included with the rest of them in the SCSS files. If there's a reason they need to be here, add a comment explaining why.
| List<Component> get leadingHeadElements => const []; | ||
|
|
||
| List<Component> get leadingBodyElements => const []; |
There was a problem hiding this comment.
If we end up keeping these getters, please add API doc comments explaining what they are/how they are used.
| } | ||
|
|
||
| :is(body.blog.dark-mode, html.dark-mode body.blog) { | ||
| header.site-header:not(.mobile-nav-open) { |
There was a problem hiding this comment.
Same here, rather than recreating custom header styles just for the blog specifically in dark mode, update the pre-existing/shared header styles to use and respect the theme-specific variables. The variables are only updated for the blog so it shouldn't result in any changes elsewhere and will be much easier to maintain. The same likely goes for other header, footer, and theme switcher styles here.
| ); | ||
| _storageSubscription = web.EventStreamProviders.storageEvent | ||
| .forTarget(web.window) | ||
| .listen((_) => _syncThemeFromStorage(updateState: true)); |
There was a problem hiding this comment.
This can be made to specifically listen to changes to the theme key:
| .listen((_) => _syncThemeFromStorage(updateState: true)); | |
| .listen((event) { | |
| if (event.key == null || event.key == 'theme') { | |
| _syncThemeFromStorage(updateState: true); | |
| } | |
| }); |
|
Thanks for the review @parlough! I've updated the PR to address all of your feedback:
Ready for another look when you have a chance! |
Adds dark mode support (
Light,Dark, andAutomatic) to the Flutter blog (flutter.dev/blog), matching the theme switcher experience ondart.devanddocs.flutter.dev:NestedThemeSwitcherin theflutter.devheader when viewing/blogpages and auto-closes the dropdown menu on selection.themeInitScriptandthemeSyncBodyScripthelpers inpackage:site_sharedso bothDashLayoutandBlogLayoutapply the storedlocalStorage['theme']preference before first paint and stay in sync across browser back/forward navigation (pageshow), cross-tabstorageevents, and live OSprefers-color-schemechanges..opaldark syntax highlighting overrides, header/footer dark styling, and diagram/icon contrast safeguards (--site-diagram-wrap-bgColor,.light-mode-visible,.dark-mode-visible,.theme-icon) scoped tobody.blogin_blog_page.scsswithout affecting otherflutter.devmarketing pages..blog-cardDOM class mutations inBlogCategoriesduring initial hydration when the defaultallview is already server-rendered.Fixes #13942