Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(card): add color property to md-card.md-THEME_NAME-theme style #7689

Closed
wants to merge 1 commit into from
Closed

fix(card): add color property to md-card.md-THEME_NAME-theme style #7689

wants to merge 1 commit into from

Conversation

ilovett
Copy link
Contributor

@ilovett ilovett commented Mar 21, 2016

Depends on #7693
Depends on #7686

Closes #7687

@ilovett
Copy link
Contributor Author

ilovett commented Mar 21, 2016

preview:

image

@devversion devversion mentioned this pull request Mar 22, 2016
Closed
devversion
devversion reviewed Mar 22, 2016
View reviewed changes
src/components/card/demoBasicUsage/index.html
@@ -1,7 +1,7 @@
<div ng-controller="AppCtrl" ng-cloak >
<md-content class="md-padding" layout-xs="column" layout="row">
<div flex-xs flex-gt-xs="50" layout="column">
<md-card>
<md-card md-theme="dark">
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, it looks a bit weird in the demos, when the first card is dark. In my opinion it would be awesome if we have a checkbox, which toggles the dark theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I like that and perhaps with multiple themes with different light/dark contrasting values

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I suggest only default and default-dark

@devversion
Copy link
Member

@ilovett Tested it locally and it doesn't work, because all card demos use the same module, and that's a bug. (See #7693).

After #7693, it works very well. So great catch 👍

@devversion devversion added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 22, 2016
@ilovett
Copy link
Contributor Author

ilovett commented Mar 22, 2016

Cool thanks, should I write "Depends on" or anything to signify that?

@devversion
Copy link
Member

@ilovett No there is no need to do that :) Just added it to your description.

@ilovett ilovett force-pushed the fix/card-theme-color branch 2 times, most recently from 948f67a to a09a3b9 Compare March 23, 2016 00:21
@ilovett
Copy link
Contributor Author

ilovett commented Mar 23, 2016

Here's new preview

image

@ilovett
Copy link
Contributor Author

ilovett commented Mar 23, 2016

Want to mention that I'm using grey backgroundPalette due to #7600.

Once #7686 is merged I will happily add different color dark themes.

@devversion devversion added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 23, 2016
@devversion
Copy link
Member

@ilovett Great, so this PR is depending on two others? Maybe you can update the description if so.

@ilovett
Copy link
Contributor Author

ilovett commented Mar 31, 2016

Rebased on upstream/master after merge of #7686 , added different background colors.

image

@ThomasBurleson ThomasBurleson self-assigned this Apr 1, 2016
@ThomasBurleson ThomasBurleson added this to the 1.1.0 milestone Apr 1, 2016
@ThomasBurleson ThomasBurleson removed this from the 1.1.0 milestone Apr 1, 2016
ilovett added a commit to ilovett/material that referenced this pull request Apr 22, 2016
@ilovett
update(demo, card): add color property to `md-card.md-THEME_NAME-th…
c1c1499 
…eme` style definition

* adds `color: '{{foreground-1}}';`

Closes angular#7687. Closes angular#7689
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@ThomasBurleson ThomasBurleson

Labels
needs: review This PR is waiting on review from the team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ilovett @devversion @ThomasBurleson