feat(navbar): Add themepicker component with lazy loaded themes #136

riavalon · 2017-03-23T19:01:41Z

Picking up the theme picker feature from where it was left off.

jelbourn

Pulled this down and tried it out- this is a good start!

Let's keep this PR scoped as-is but omit the theme-picker from the navbar until we get these follow-ups done:

Show the currently selected theme in the picker (similar to fonts.google.com)
Use inline SVGs instead of img / background-image so that we can manipulate their colors (could use md-icon for this).
Fix text and background color on all pages when switching to a dark theme
Remember selected theme in local storage (including updating on change)
Load different highlight.js files based on whether we're in a light or dark theme

jelbourn · 2017-04-08T16:55:26Z

src/app/shared/style-manager/style-manager.ts

+   * @param key The key for the slot whose element we want.
+   * @param create Whether to create the element if it doesn't exist.
+   * @returns {HTMLLinkElement} The `<link.` element.
+   * @private


Omit @private and type information from JsDoc since that's captured in the TypeScript code.

jelbourn · 2017-04-08T17:09:38Z

src/app/shared/style-manager/style-manager.ts

+      document.head.appendChild(linkEl);
+    }
+    return linkEl as HTMLLinkElement;
+  }


Rather than having one function perform two behaviors with an option, I generally prefer breaking it up into separate functions. Also, most of these can be module-level functions since they are pure functions, e.g.

class StyleManager { setStyle(key: string, href: string) { getLinkElementForKey(key).setAttribute('href', href); } removeStyle(key: string) { const existingLinkElement = getExistingLinkElementByKey(key); if (existingLinkElement) { document.head.removeChild(existingLinkElement); } } } function getLinkElementForKey(key: string) { return getExistingLinkElementByKey(key) || createLinkElementWithKey(key); } function getExistingLinkElementByKey(key: string) { return document.head.querySelector(`link[rel="stylesheet"].${getClassNameForKey(key)}`); } function createLinkElementWithKey(key: string) { const linkEl = document.createElement('link'); linkEl.setAttribute('rel', 'stylesheet'); linkEl.classList.add(getClassNameForKey(key)); document.head.appendChild(linkEl); return linkEl; } function getClassNameForKey(key: string) { return `style-manager-${key}`; }

jelbourn · 2017-04-08T17:11:00Z

src/app/shared/theme-chooser/theme-chooser.scss

+$theme-chooser-accent-stripe-size: 6px;
+
+
+.theme-chooser-menu {


Prefix css classes with docs-

jelbourn · 2017-04-08T17:13:36Z

src/app/shared/theme-chooser/theme-chooser.ts

+import {Component, ViewEncapsulation, ChangeDetectionStrategy} from '@angular/core';
+import {StyleManager} from '../style-manager/style-manager';
+
+@Component({


We should make the theme chooser aria-hidden="true" and ensure that the button is not in the tab order since a screen-reader user won't get any benefit from changing the app colors.

jelbourn · 2017-04-08T17:15:14Z

src/app/shared/theme-chooser/theme-chooser.ts

+    {
+      primary: '#673AB7',
+      accent: '#FFC107',
+      href: 'assets/deeppurple-amber.css'


We should omit assets/ on each entry and add it in the function call.

jelbourn · 2017-04-08T17:24:55Z

angular-cli.json

+        {"input": "assets/pink-bluegrey.css", "lazy": true},
+        {"input": "assets/deeppurple-amber.css", "lazy": true},
+        {"input": "assets/indigo-pink.css", "lazy": true},
+        {"input": "assets/purple-green.css", "lazy": true}


Can these be referenced directly from node_modules rather than copying them to assets?

jelbourn · 2017-04-08T17:27:09Z

src/app/shared/theme-chooser/theme-chooser.scss

+
+  .theme-chooser-accent {
+    position: absolute;
+    bottom: $theme-chooser-accent-stripe-size;


How do you feel about the accent stripe? I'm not particularly fond of it.

jelbourn

I pulled down the PR and tried it out- looks great!

jelbourn · 2017-05-31T21:13:06Z

src/app/app-module.ts

+    Footer,
+  ],
+  schemas: [
+    CUSTOM_ELEMENTS_SCHEMA,


CUSTOM_ELEMENTS_SCHEMA ideally shouldn't be here; why was this needed?

jelbourn · 2017-05-31T21:14:07Z

src/app/app-module.ts

@@ -44,10 +54,12 @@ import {ComponentPageHeader} from './pages/component-page-header/component-page-
    MaterialModule,
    MdNativeDateModule,
    routing,
+    InlineSVGModule,


InlineSvgModule (camelCase even for acronyms)

jelbourn · 2017-05-31T21:14:33Z

src/app/material-docs-app.ts

@@ -1,6 +1,8 @@
 import {Component, ViewEncapsulation} from '@angular/core';
 import {Router, NavigationStart} from '@angular/router';

+// import {ThemeStorage} from './shared/theme-chooser/theme-storage/theme-storage';


Commented line

jelbourn · 2017-05-31T21:14:42Z

src/app/material-docs-app.ts

+
+  // public _setIsDarkTheme(theme) {
+  //   this.isDarkTheme = theme ? theme.isDark : this.isDarkTheme;
+  // }


Commented code

jelbourn · 2017-05-31T21:16:22Z

src/app/pages/component-category-list/component-category-list.scss

-  background-size: contain;
-  background-repeat: no-repeat;
-  background-position: center;
+:host /deep/ .docs-component-category-list-card-image svg {


I'd like to avoid /deep/ completely, instead preferring to add a global style (or, barring that, turning off view encapsulation for the component)

jelbourn · 2017-05-31T21:25:10Z

src/app/shared/svg-viewer/svg-builder.ts

+const Color = require('color');
+
+
+export interface IThemeColors {


We prefer not to prefix interfaces with I (here and elsewhere)

jelbourn · 2017-05-31T21:25:46Z

src/app/shared/svg-viewer/svg-builder.ts

+
+import {IDocsSiteTheme} from '../theme-chooser/theme-storage/theme-storage';
+
+const Color = require('color');


What's this require doing here?

jelbourn · 2017-05-31T21:29:43Z

src/app/shared/svg-viewer/svg-viewer.ts

+
+  public ngAfterViewInit() {
+    if (this.currTheme) {
+      setTimeout(this.swapTheme.bind(this, this.currTheme));


Prefer arrow functions to using bind

jelbourn · 2017-05-31T21:30:20Z

src/app/shared/theme-chooser/theme-chooser.html

+        <div class="docs-theme-chooser-swatch">
+          <md-icon class="docs-theme-chosen-icon" *ngIf="currentTheme === theme">check_circle</md-icon>
+          <div class="docs-theme-chooser-primary" [style.background]="theme.primary"></div>
+          <!--<div class="docs-theme-chooser-accent" [style.background]="theme.accent"></div>-->


Commented code

jelbourn · 2017-05-31T21:32:45Z

src/app/shared/theme-chooser/theme-chooser.html

+  <md-icon>format_color_fill</md-icon>
+</button>
+
+<md-menu class="docs-theme-chooser-menu" #themeMenu="mdMenu" x-position="before">


Add a TODO here like

jelbourn

LGTM

masonlr · 2019-03-01T12:48:45Z

@jelbourn I'm looking at theme-picker.ts and can't see how the default theme (isDefault: true property in themes) is honoured when you first load the page.

installTheme() is called from the constructor with a theme argument if there is a theme in local storage or a theme is supplied by query parameters (otherwise it's called with a null argument). If it's the first time you're visiting the page, neither of these may be present.

Am I missing something?

riavalon assigned jelbourn Mar 23, 2017

googlebot added the cla: yes label Mar 23, 2017

riavalon force-pushed the theme-picker branch 4 times, most recently from 6857c10 to 7f6cd6c Compare March 30, 2017 18:02

jelbourn reviewed Apr 8, 2017

View reviewed changes

riavalon force-pushed the theme-picker branch 3 times, most recently from 46e1553 to 74eb084 Compare April 13, 2017 17:24

riavalon force-pushed the theme-picker branch 4 times, most recently from d586c1e to f9b7669 Compare April 24, 2017 15:05

riavalon force-pushed the theme-picker branch 4 times, most recently from 9dcc56b to 1cfa71f Compare April 26, 2017 15:59

riavalon force-pushed the theme-picker branch 3 times, most recently from ee4b628 to db8f833 Compare May 26, 2017 18:22

jelbourn reviewed May 31, 2017

View reviewed changes

riavalon force-pushed the theme-picker branch 3 times, most recently from 798538e to 1649a41 Compare June 6, 2017 19:39

feat(navbar): Add themepicker component with lazy loaded themes

05fed02

riavalon force-pushed the theme-picker branch from 1649a41 to 05fed02 Compare June 15, 2017 17:50

jelbourn approved these changes Jun 15, 2017

View reviewed changes

jelbourn merged commit f5478ed into angular:master Jun 15, 2017

Splaktar mentioned this pull request Jul 10, 2017

Implement theme switcher #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(navbar): Add themepicker component with lazy loaded themes #136

feat(navbar): Add themepicker component with lazy loaded themes #136

riavalon commented Mar 23, 2017 •

edited

Loading

jelbourn left a comment •

edited by riavalon

Loading

jelbourn Apr 8, 2017

jelbourn Apr 8, 2017

jelbourn Apr 8, 2017

jelbourn Apr 8, 2017

jelbourn Apr 8, 2017

jelbourn Apr 8, 2017

jelbourn Apr 8, 2017

jelbourn left a comment

jelbourn May 31, 2017 •

edited

Loading

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn May 31, 2017

jelbourn left a comment

masonlr commented Mar 1, 2019 •

edited

Loading

		$theme-chooser-accent-stripe-size: 6px;


		.theme-chooser-menu {

		const Color = require('color');


		export interface IThemeColors {


		import {IDocsSiteTheme} from '../theme-chooser/theme-storage/theme-storage';

		const Color = require('color');

feat(navbar): Add themepicker component with lazy loaded themes #136

feat(navbar): Add themepicker component with lazy loaded themes #136

Conversation

riavalon commented Mar 23, 2017 • edited Loading

jelbourn left a comment • edited by riavalon Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

masonlr commented Mar 1, 2019 • edited Loading

riavalon commented Mar 23, 2017 •

edited

Loading

jelbourn left a comment •

edited by riavalon

Loading

jelbourn May 31, 2017 •

edited

Loading

masonlr commented Mar 1, 2019 •

edited

Loading