Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

feat(demos): Add theme switcher to theme demo page #1975

Merged
merged 30 commits into from
Jan 19, 2018

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Jan 17, 2018

This PR also adds hot reloading of stylesheets that have a data-hot attribute.

Currently, only the theme demo page has a <link rel="stylesheet" href="..." data-hot> element, but I plan to create follow-up PRs to add hot swapping to the remaining demo pages. Hot swapping was broken when we switched to CSS files in #1916.

When Webpack starts recompiling our Sass, the theme demo page displays a linear-progress bar in the toolbar until compilation finishes, at which point it automatically swaps in the new stylesheet.

I tested this PR in:

  • Chrome 63 (desktop and mobile)
  • Firefox 57 (desktop)
  • Safari 11 (desktop and mobile)
  • IE 11
  • Edge 14

commit 1e8970f
Author: Matty Goo <[email protected]>
Date:   Wed Jan 17 09:19:54 2018 -0800

    chore(text-field): moved textarea sass into private mixins (#1942)

commit bffadd3
Author: Andrew C. Dvorak <[email protected]>
Date:   Wed Jan 17 09:09:56 2018 -0800

    style(demos): Clean up theme demo Sass and HTML (#1973)

    - Remove unused code
    - IE 11 compatible property values (`unset` -> `auto`)
    - Fix duplicate IDs
    - Use `--stroked` buttons instead of `--raised` for checkbox demo
    - Rename CSS classes to be more BEM-y
    - Rename and reorganize Sass demo vars
    - Reword text-field labels and helper text for clarity
    - Remove `getAll()`
    - Inline some JS vars

commit 3a1786f
Author: Dominic Carretto <[email protected]>
Date:   Wed Jan 17 11:37:03 2018 -0500

    fix(slider): Add MDCSliderFoundation export (#1959)

commit 815eade
Author: Simon Olofsson <[email protected]>
Date:   Wed Jan 17 17:36:07 2018 +0100

    docs(menu): Remove obsolete `mdc-simple-menu--open-from` classes. (#1927)

commit 6078784
Author: Chafic Najjar <[email protected]>
Date:   Wed Jan 17 18:26:00 2018 +0200

    docs: Rewrite all instances of "MDC-Web" as "MDC Web" (#1960)

    Resolves #1924

commit e87c110
Author: pndewit <[email protected]>
Date:   Wed Jan 17 17:23:03 2018 +0100

    docs(drawer): Fix missing link to temporary drawer demo (#1922)

commit 003dff4
Author: Chafic Najjar <[email protected]>
Date:   Wed Jan 17 18:20:35 2018 +0200

    docs: Fix broken links to AngularJS's Git commit guidelines (#1888)
@codecov-io
Copy link

codecov-io commented Jan 17, 2018

Codecov Report

Merging #1975 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1975   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files          84       84           
  Lines        3717     3717           
  Branches      485      485           
=======================================
  Hits         3696     3696           
  Misses         21       21

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5e0464...2029047. Read the comment docs.

@acdvorak acdvorak changed the title feat(demos): Add theme switcher and RTL toggle to theme demo page feat(demos): Add theme switcher to theme demo page Jan 17, 2018
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

tumblr_nr5r5y8u4f1uay2eho1_400

demos/dom.js Outdated
* @return {!Array<!Element>}
*/
export function getAll(query, opt_root) {
opt_root = opt_root || document;
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop opt_ prefix and inline as default parameter? getAll(query, root = document)

The opt_ prefix is discouraged now in favor of default parameters. (See reference)

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

* @return {?Element}
* @protected
*/
getElementById_(id, opt_root) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this more complicated than simply calling document.getElementById?

For that matter, are these various root parameters ever passed anything other than document in practice, or do we have some cases of YAGNI in here?

Also, same idea here and below RE opt_root -> root = ...

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

this.flashCurrentSection_();
}

// TODO(acdvorak): Replace page URL hash with current section on scroll
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder that this TODO is here, whether or not you want to deal with it right now; given that we have links in each heading I don't think it's critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed TODO. Thanks!

@@ -61,10 +66,35 @@ body {
background-color: $mdc-theme-background;
}

figure {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be redundant of styles 10 lines above this. Remove this or remove those?

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

- Remove `opt_` prefix from function arg names
- Use default values (except in uri.js and theme-loader.js, which are not transpiled)
- Remove unneeded `|undefined` from optional param JSDoc
- Rename var for clarity
@acdvorak acdvorak merged commit 4f89819 into master Jan 19, 2018
@acdvorak acdvorak deleted the experimental/theme/demo-v2 branch January 19, 2018 22:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants