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(theme): remove dark theme #2169

Merged
merged 8 commits into from
Feb 3, 2018
Merged

Conversation

moog16
Copy link
Contributor

@moog16 moog16 commented Feb 2, 2018

BREAKING CHANGE: deleted mdc-theme-light-or-dark and mdc-theme-dark

@codecov-io
Copy link

codecov-io commented Feb 2, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2169   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files          90       90           
  Lines        3843     3843           
  Branches      497      497           
=======================================
  Hits         3810     3810           
  Misses         33       33

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 d2e53e8...e15c709. Read the comment docs.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

  • There is a "Dark Themes" section in docs/theming.md that we should re-word or remove, and make sure there are no longer references to the mixin/classes we're removing
  • Remove reminder about documenting dark theme support in readme_standards.md

@@ -47,32 +43,6 @@ fieldset {
border: 0;
}

// TODO mgoo: remove these dark theme when complete with removing other components
Copy link
Contributor

Choose a reason for hiding this comment

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

Also remove $dark-button-color which is no longer used after this is removed

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Nits on replacement docs for theming.md

docs/theming.md Outdated
```

Alternatively, you can set your entire page (or a portion of it) to a dark theme by using the `mdc-theme--dark` class:
Html Markup would then look like:
Copy link
Contributor

Choose a reason for hiding this comment

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

"Then apply the custom class to some of our buttons, like so:"

docs/theming.md Outdated

## Dark Themes
We also have provided a way to customize on a per component basis. We realize that you may want a button with a
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be okay to focus on a single example here, and use mdc-button-filled-accessible which automatically picks ink color based on fill color for best contrast.

Lead off with something like, "For example, let's say you want to make particular buttons stand out by changing their fill color."

docs/theming.md Outdated
## Dark Themes
We also have provided a way to customize on a per component basis. We realize that you may want a button with a
blue background and another with a red background on the same page. Each component's package is documented with all
our provided sass mixins and what arguments to pass them. For example, here is a link to our [Button Sass Mixins docs](https://github.com/material-components/material-components-web/tree/master/packages/mdc-button#advanced-sass-mixins).
Copy link
Contributor

Choose a reason for hiding this comment

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

  • "all our provided" -> "all available"
  • "what arguments to pass them" -> "what parameters they accept"
  • Use a relative link to the button docs (../packages/mdc-button#advanced-sass-mixins should work) and rephrase to "See the MDC Button Sass mixins for example." (with link around "MDC Button Sass mixins")

docs/theming.md Outdated
Beyond what we've covered in this document so far, there's also the concept of a _dark theme_. All MDC Web components have
been designed to work with both light themes (that assume a light-colored background) and dark themes (with dark-colored
backgrounds), but the default is always light.
If we continue the example stated above, here is what the sass would like:
Copy link
Contributor

Choose a reason for hiding this comment

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

Rephrase to "Here is an example of how we could add styles to change the color of some buttons:"

(For some reason when I read "the example stated above", I was thinking it was referring to further above this section of the document, rather than just a paragraph ago)

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

Some minor comments about wording in theming.md. We usually try to maintain a consistent "voice" across all of our docs 🙂

docs/theming.md Outdated

## Dark Themes
For example, let's say you want to make particular
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rephrase this paragraph to something like:

Most MDC Web components provide a set of Sass mixins to customize their appearance,
such as changing the fill color, ink color, stroke width, etc.
These mixins are documented in each component's README file
(e.g., the [Button readme](../packages/mdc-button/README.md#advanced-sass-mixins)).

And line wrap at 120 chars 🙂

docs/theming.md Outdated
```

Alternatively, you can set your entire page (or a portion of it) to a dark theme by using the `mdc-theme--dark` class:
Then apply the custom class to some of our buttons, like so:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Rephrase to "Then apply the custom class to the button elements:"

docs/theming.md Outdated
<button class="mdc-button mdc-button--raised mdc-button--theme-dark">
Raised dark button
</button>
```css
Copy link
Contributor

Choose a reason for hiding this comment

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

s/css/scss/

docs/theming.md Outdated
@@ -430,37 +430,25 @@ Since our cards only contain text and no components, let's keep it simple for no
> Note: in the future we plan to provide a Javascript utility method for changing all derived colors and making this
use-case easier.

## Custom Sass Mixins
Copy link
Contributor

Choose a reason for hiding this comment

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

Custom Sass Mixins -> Custom Themes

docs/theming.md Outdated
Beyond what we've covered in this document so far, there's also the concept of a _dark theme_. All MDC Web components have
been designed to work with both light themes (that assume a light-colored background) and dark themes (with dark-colored
backgrounds), but the default is always light.
Here is an example of how we could add styles to change the color of some buttons:
Copy link
Contributor

Choose a reason for hiding this comment

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

For example, to change the fill color of a button and automatically select an accessible ink color,
simply call the `mdc-button-filled-accessible` mixin inside a custom CSS class:

@moog16
Copy link
Contributor Author

moog16 commented Feb 3, 2018

@acdvorak ya sounds a lot better than what I had :)

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

LGTM!

@moog16 moog16 merged commit 13b5605 into master Feb 3, 2018
@moog16 moog16 deleted the chore/theme/remove-dark-theme branch February 3, 2018 01:15
@langan
Copy link

langan commented Feb 8, 2018

Could you give some insight as to why this was removed?

Is there an alternative method now to style a component using the dark theme?

@moog16
Copy link
Contributor Author

moog16 commented Feb 8, 2018

@langan - Dark theme required a lot of maintenance and bloated our scss. We have been investing time in creating sass mixins to customize our components. This gives the developer more flexibility to customizing components. Please refer to https://github.com/material-components/material-components-web/blob/master/docs/theming.md#custom-themes for more.

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

5 participants