In this use case, this callout/alert section looses it's visual impact - I'd test a higher contrast value for $theme-hover-background-color such as black (going darker increases the contrast of the light text on top).

This is an example of the contrast being lost on hover - if the ratio can meet WCAG AAA (7.0 contrast) that'd meet best practices for our small text.

Spot check contrast here since this is the primary site navigation -

I think the aria-label here should be either "Switch to light mode" or "Switch to dark mode" (based on the active theme).

Since the icon also conveys valuable information of if the site is currently in light or dark mode, I'd recommend adding "sun icon" and "moon icon" to aria-labels

Rather than using any, it would be good to match the interface of the actual class here

Just ThemePicker should be sufficient

I don't think this theme data structure is necessary for angular.io. On the material site, we need these fields for additional behavior. For example, we need a display name because we show the options in a dropdown menu, which is not necessary here.

I think this can be simplified down to a single string value, theme: 'light' | 'dark, with the icon based on that string value. I would also generate the applied CSS class based on that value like `${activeTheme}-theme`

This is more a style preference, but I would use activeTheme over currentTheme

Since this is only doing light/dark, I would make this toggleTheme, dropping the parameter.

Maybe just 'aio-theme'

I would just do ThemeStorage

This method doesn't appear to ever be called in the app so it can probably be dropped

This is going to include all of the CSS for light and dark themes at the same time, which we don't particularly want. On the material site, we swap out the href on the link to point to a different css file based on the theme preference so that we only load one theme at a time.

One concern with this approach is that, on slower devices, you can sometimes see a flash of the default theme before your preferred theme is loaded, since that isn't resolved until Angular has bootstrapped.

If you go network panels and slow it down to 3g for https://material.angular.io. You'll notice the same issue happening.

Yeah, it's something we decided we were okay with on the material site, but other folks would have to weigh in on angular.io. If you didn't have the saved preference, you could use the media attribute of the link element to target prefers-color-scheme. But with the preference, you'd need a small snippet to read/set the href before anything else happened.

If the sass is all compiled to a single css file like it is here, then I don't know if you would notice the change in styling. If anything you would notice it if you tried the approach of href and dynamically loading another css file (as I have ran into that issue in other projects). I'm trying to see if slowing it to 3g does cause what you talked about, but I couldn't reproduce it.
Media attribute is a bit more risky as I wanted the user to have the option to opt out if they don't want it.

The issue with using CSS classes to toggle the theme isn't that you'll get a flash of the default theme, it's that the browser has to download a nontrivial amount of additional CSS that they aren't using. I don't know the total size offhand, but we do our best to keep the payload size of angular.io as small as we can, and doubling the theme styles would be something we'd want to avoid.

Yeah if the concern is size, then my PR is not gonna meet the standard. It's meant to not infringe the current code as much as possible without affecting the feature. Feel free to close it, and maybe someone can tackle the refactoring at some point.

Looking through this file, I don't think we can move forward with it in its current form. The biggest problem is that it's relying on global styles to override component-specific styles with greater specificity. These global overrides, though, are disconnected from the code they're affecting. This disconnect will make any maintenance going forward exceptionally difficult. This is compounded by the fact that this single file has implicit knowledge of every hard-coded color style in the app. Another side-effect here is that it becomes very easy to introduce a style into this file that accidentally affects more than it intends, since anything in this file affects the entire app all at once.

Another issue is that the app ends up delivering the hard-coded and the dark-mode overrides. The end result is that we're delivering more CSS than really necessary. This also has the side-effect of making the code harder to debug in devtools since there's more going on with any particular element.

Another issue is that these overrides seem to target the internals of Angular Material components in many cases, which is rather brittle since it's tied into the exact DOM structure of the component today. All of the Angular Material components support dark themes, so I'm guess the problem here is that those styles are already being overridden elsewhere in the app here without doing through their theme mixns?

There are a few general approaches we could go about resolving these issues. The first is what Angular Material does: splitting color styles for the app into separate Sass files, and combining those into a theme CSS file, which can then be dynamically loaded/swapped. I suspect this would be a large-ish undertaking for aio since you'd have to do a nontrivial refactoring for every component in the app.

The second approach would be to eliminate the hard-coded colors across the app and replace them with a fixed set of theme CSS classes derived from the Sass theme. For example, if a component current has something like

.content-projection-example {
  color: darkgray;
}

You would eliminate this style and instead apply a predefined CSS class like .theme-foreground-color to the element in the template. This class would be defined in the theme CSS file, which would be dynamically loaded based on the light/dark preference, something like

.theme-foreground-color {
  color: mat-color($foreground-palette, text);
}

I think this would also be a large-ish undertaking that touches a lot of the app.

The third approach would be to use CSS Variables, redefining the hard-coded colors within each component to be based on a limited set CSS Variables, which are in turn based off values coming from the theme in Sass. The blocker here, though, is that CSS Variables aren't supported in IE11, which we still support for Angular. This will become a much more attractive option in the future, but for now I don't think we can feasibly adopt it. (as a side note, you unfortunately wouldn't be able to use Sass variables to implement this approach due to the way Angular bundles component CSS into JavaScript).

At the very least I think @gkalpak @IgorMinar and @mgechev would want to weigh in on this. I'm biased, but I think that using the same approach as Angular Material is the "best" end result here, though it's likely a fair amount of work to get there.

Yeah I understand what you mean. I decided to go with this approach, as I am not sure if that level of refactoring would bring that much value relative to the feature itself, especially with this being my first time touching this repo.
What about a fourth option to make all the corresponding sass files take in the theme input, similar to how custom_styles does it? Would that work? Probably the only other way I can think of that reduces significant refactoring.

That unfortunately would be the same as trying to use Sass variables, which I mentioned in the block for the third approach. The problem here is that Angular bundles a component's CSS into its JS output. This lets the styles participate in the JS module system (i.e., you only get the styles for a component when you use it), but also means that you can't swap out the style at runtime.

This @mixin file is quite a bit of custom overriding of the current dark theming included in Material

There should be a cleaner way to use material theming and not need this amount of overriding - making use of things like defining theme scss variables: $mat-dark-theme-background and $mat-dark-theme-foreground