Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions aio/src/app/app.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
<mat-icon svgIcon="logos:github"></mat-icon>
</a>
</div>
<theme-picker></theme-picker>
</mat-toolbar-row>
</mat-toolbar>

Expand Down
17 changes: 13 additions & 4 deletions aio/src/app/shared/shared.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,27 @@ import { NgModule } from '@angular/core';
import { CommonModule } from '@angular/common';
import { SearchResultsComponent } from './search-results/search-results.component';
import { SelectComponent } from './select/select.component';
import { MatButtonModule } from '@angular/material/button';
import { MatIconModule } from '@angular/material/icon';
import { ThemePickerComponent } from './theme-picker/theme-picker.component';
import { ThemeStorageService } from './theme-picker/theme-storage/theme-storage.service';

@NgModule({
imports: [
CommonModule
CommonModule,
MatButtonModule,
MatIconModule
],
exports: [
SearchResultsComponent,
SelectComponent
SelectComponent,
ThemePickerComponent
],
declarations: [
SearchResultsComponent,
SelectComponent
]
SelectComponent,
ThemePickerComponent
],
providers: [ThemeStorageService]
})
export class SharedModule {}
3 changes: 3 additions & 0 deletions aio/src/app/shared/theme-picker/theme-picker.component.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<button mat-icon-button aria-label="Theme switcher button" (click)="currentTheme.name === 'night-theme' ? selectTheme('ng-io-theme') : selectTheme('night-theme')">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

<mat-icon>{{currentTheme.name === 'night-theme' ? themes[0].icon : themes[1].icon}}</mat-icon>
</button>
24 changes: 24 additions & 0 deletions aio/src/app/shared/theme-picker/theme-picker.component.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import {async, TestBed} from '@angular/core/testing';
import { ThemePickerComponent } from './theme-picker.component';
import { ThemeStorageService } from './theme-storage/theme-storage.service';


describe('ThemePicker', () => {
let themeStorageService: any;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


beforeEach(async(() => {
themeStorageService = jasmine.createSpyObj('themeStorageService', ['getStoredThemeName', 'storeTheme']);
TestBed.configureTestingModule({
declarations: [ThemePickerComponent],
providers: [ { provide: ThemeStorageService, useValue: themeStorageService } ]
}).compileComponents();
}));

it('should install theme based on name', () => {
const fixture = TestBed.createComponent(ThemePickerComponent);
const component = fixture.componentInstance;
const name = 'night-theme';
component.selectTheme(name);
expect(themeStorageService.storeTheme).toHaveBeenCalledWith(component.themes[1]);
});
});
53 changes: 53 additions & 0 deletions aio/src/app/shared/theme-picker/theme-picker.component.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import {
ChangeDetectionStrategy,
Component,
ViewEncapsulation,
} from '@angular/core';
import {DocsSiteTheme, ThemeStorageService} from './theme-storage/theme-storage.service';

@Component({
selector: 'theme-picker',
templateUrl: 'theme-picker.component.html',
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None
})
export class ThemePickerComponent {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just ThemePicker should be sufficient

readonly themes: DocsSiteTheme[] = [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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`

{
primary: '#1976d2',
accent: '#e91e63',
displayName: 'Light Theme',
name: 'ng-io-theme',
icon: 'light_mode'
},
{
primary: '#00796b',
accent: '#80cbc4',
displayName: 'Night Theme',
name: 'night-theme',
isDark: true,
icon: 'dark_mode'
},
];

readonly defaultTheme: DocsSiteTheme = this.themes[0];
currentTheme: DocsSiteTheme = this.themes[0];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


constructor(private themeStorageService: ThemeStorageService) {
const themeName = this.themeStorageService.getStoredThemeName();
if (themeName) {
this.selectTheme(themeName);
}
}

selectTheme(themeName: string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

const theme = this.themes.find(currentTheme => currentTheme.name === themeName);
this.currentTheme = theme || this.defaultTheme;
if (this.currentTheme.name === this.defaultTheme.name) {
document.body.classList.value = '';
} else {
document.body.classList.value = this.currentTheme.name;
}
this.themeStorageService.storeTheme(this.currentTheme);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import {DocsSiteTheme, ThemeStorageService} from './theme-storage.service';


const testStorageKey = ThemeStorageService.storageKey;
const testTheme: DocsSiteTheme = {
primary: '#000000',
accent: '#ffffff',
name: 'test-theme',
icon: 'test-icon'
};

describe('ThemeStorage Service', () => {
const service = new ThemeStorageService();
const getCurrTheme = () => window.localStorage.getItem(testStorageKey);
const secondTestTheme = {
primary: '#666666',
accent: '#333333',
name: 'other-test-theme',
icon: 'other-test-icon'
};

beforeEach(() => {
window.localStorage[testStorageKey] = testTheme.name;
});

afterEach(() => {
window.localStorage.clear();
});

it('should set the current theme name', () => {
expect(getCurrTheme()).toEqual(testTheme.name);
service.storeTheme(secondTestTheme);
expect(getCurrTheme()).toEqual(secondTestTheme.name);
});

it('should get the current theme name', () => {
const theme = service.getStoredThemeName();
expect(theme).toEqual(testTheme.name);
});

it('should clear the stored theme data', () => {
expect(getCurrTheme()).not.toBeNull();
service.clearStorage();
expect(getCurrTheme()).toBeNull();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import {Injectable} from '@angular/core';

export interface DocsSiteTheme {
name: string;
displayName?: string;
accent: string;
primary: string;
isDark?: boolean;
icon: string;
isDefault?: boolean;
}


@Injectable()
export class ThemeStorageService {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would just do ThemeStorage

static storageKey = 'docs-theme-storage-current-name';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just 'aio-theme'


storeTheme(theme: DocsSiteTheme) {
try {
window.localStorage[ThemeStorageService.storageKey] = theme.name;
} catch { }
}

getStoredThemeName(): string | null {
try {
return window.localStorage[ThemeStorageService.storageKey] || null;
} catch {
return null;
}
}

clearStorage() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

try {
window.localStorage.removeItem(ThemeStorageService.storageKey);
} catch { }
}
}
Loading