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

fix(button): Default to primary color #1356

Merged
merged 3 commits into from
Sep 28, 2017
Merged

Conversation

kfranqueiro
Copy link
Contributor

@kfranqueiro kfranqueiro commented Sep 26, 2017

Fixes #1345.

I should point out a couple of caveats with this approach, which differs from the prior approach of providing --primary and --accent (now AKA secondary) as opt-in classes...

  • By setting a hard default to primary, light-vs-dark considerations are dropped, and in general, contrast considerations will be left to the user when customizing theme if customizing via CSS variables.
    • I'm a little concerned especially about flat buttons using primary for text/ink color, in cases where the primary color is relatively light.
  • Current versions of Edge with (buggy) CSS variable support will have problems displaying this. I'm pondering looking into mdc-button in Edge browser is colorless #1102 to see if this can be addressed.

@kfranqueiro
Copy link
Contributor Author

Added another commit to address contrast issues with dark theme (perfect example of one of the caveats above)... I'll definitely want to talk over dark theme with designers some more as to the direction we should take that.

image

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.

Thanks for doing this Ken. We'll discuss dark theme more in the next couple months

@kfranqueiro kfranqueiro merged commit 0b808b8 into master Sep 28, 2017
@kfranqueiro kfranqueiro deleted the fix/button-inherit-theme branch September 28, 2017 14:23
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