Skip to content
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

chore(demos): Add example themes & load theme from URL #1967

Merged
merged 7 commits into from
Jan 17, 2018

Conversation

acdvorak
Copy link
Contributor

To test, visit the following URL:

http://localhost:8080/theme/index.html?theme=dark

The theme param must be one of baseline, black, dark, white, or yellow.

This only works on the theme demo page; all other demo pages are unaffected.

To test, visit the following URL:

http://localhost:8080/theme/index.html?theme=dark

The `theme` param must be one of `baseline`, `black`, `dark`, `white`, or `yellow`.
Copy link
Contributor

@lynnmercier lynnmercier left a comment

Choose a reason for hiding this comment

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

I don't think I understand all the complexity of theme-loader.js and uri.js ... are you doing some security protection? (Can we just use a Node module instead of uri.js?? Something open source?)


@import "../../packages/mdc-theme/color-palette";

$demo-toolbar-progress-bar-color: secondary-light;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a TODO here to remove this once all components use baseline as a default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@import "../../packages/mdc-theme/color-palette";

$mdc-theme-primary: $material-color-grey-900;
$mdc-theme-secondary: $material-color-light-green-a700;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a TODO here to call Sass mixins instead of using mdc-theme-primary and mdc-theme-secondary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


$mdc-theme-primary: $material-color-amber-300;
$mdc-theme-secondary: $material-color-pink-400;
$mdc-theme-background: $material-color-grey-900;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a TODO here to call Sass mixins instead of using mdc-theme-primary, mdc-theme-secondary, and mdc-theme-background

Also, call the Sass mixins for the checkbox so that the demo example on theme=dark is a yellow checkbox (or pink, whatevs)

And call the Sass mixins for the radio button, the select, the tabs, and the text field (most of those mixins are written, so just use whatever is there)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Select and Tabs don't have theme mixins yet, so I left a TODO for those.

@import "../../packages/mdc-theme/color-palette";

$mdc-theme-primary: #fff;
$mdc-theme-secondary: $material-color-orange-500;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a TODO here to call Sass mixins instead of using mdc-theme-primary and mdc-theme-secondary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@import "../../packages/mdc-theme/color-palette";

$mdc-theme-primary: $material-color-yellow-500;
$mdc-theme-secondary: $material-color-blue-500;
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a TODO here to call Sass mixins instead of using mdc-theme-primary and mdc-theme-secondary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lynnmercier lynnmercier self-assigned this Jan 16, 2018
@acdvorak
Copy link
Contributor Author

I don't think I understand all the complexity of theme-loader.js and uri.js ... are you doing some security protection? (Can we just use a Node module instead of uri.js?? Something open source?)

theme-loader.js and uri.js are needed to prevent XSS attacks. Reading from the query string and injecting it onto the page using document.write() is the only way I know of to prevent FOUC, but blindly inserting untrusted user input into the page is super dangerous. A malicious actor could craft a special URL that does something evil if we don't carefully sanitize the query string values.

I considered using existing Node modules for this, but A) I don't trust them to be secure, and B) they're all big and bloated, and since this script is going in the <head>, it needs to be small and fast to avoid blocking the page over slow network connections.

I tried to follow the patterns employed by Closure lib's SafeHtml class, which was specifically designed to make it difficult for attackers to exploit XSS vulnerabilities.

Copy link
Contributor

@lynnmercier lynnmercier left a comment

Choose a reason for hiding this comment

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

LGTM, two comments left


@import "../../packages/mdc-theme/color-palette";

// TODO(acdvorak): Use theme mixin(s) instead
Copy link
Contributor

Choose a reason for hiding this comment

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

don't put your name here...that implies you are going to do all this work very soon. While in reality it will probably be many people over several months.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@include mdc-theme-prop(color, text-primary-on-dark);
}

// TODO(acdvorak): Call theme mixins for mdc-select and mdc-tab-bar.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can call some mixins for tabs. they are under mdc-tabs/tab/_mixins: mdc-tab-ink-color or mdc-tab-label-ink-color and mdc-tab-icon-ink-color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how I missed those. Thanks!

@acdvorak acdvorak merged commit f399384 into master Jan 17, 2018
@acdvorak acdvorak deleted the chore/demos/theme-loader branch January 17, 2018 04:09
acdvorak added a commit that referenced this pull request Jan 17, 2018
commit f399384
Author: Andrew C. Dvorak <acdvorak@gmail.com>
Date:   Tue Jan 16 20:09:43 2018 -0800

    chore(demos): Add example themes & load theme from URL (#1967)

    To test, visit the following URL:

    http://localhost:8080/theme/index.html?theme=dark

    The `theme` param must be one of `baseline`, `black`, `dark`, `white`, or `yellow`.

    This only works on the theme demo page; all other demo pages are unaffected.

commit 1dae53c
Author: Will Ernest <34519388+williamernest@users.noreply.github.com>
Date:   Tue Jan 16 19:14:54 2018 -0800

    feat(text-field): Move text-field outline colors to mixins (#1963)

    * feat(text-field): Move color mixins for outline/helper text. Update demos/docs
    Breaking Change: Moves color customization of the outline text-field to SASS mixins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants