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): Remove background-color from raised buttons on dark theme #833

Closed
wants to merge 1 commit into from

Conversation

unascribed
Copy link

@unascribed unascribed commented Jun 15, 2017

See: #819

This runs contrary to the guidelines, but considering MDC gives full control of button color through class names (and CSS is very flexible), it's likely a non-issue. This also fixes the text color being wrong when a light primary color is used.

Fair warning: I tested this by deleting the directive with Chrome devtools. I'm 90% sure my change here has the desired effect, but I haven't yet had a chance to test a fresh compile.

See: material-components#819

This runs contrary to the guidelines, but considering MDC gives full control of button color through class names (and CSS is very flexible), it's likely a non-issue. This also fixes the text color being wrong when a light primary color is used.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@unascribed
Copy link
Author

Signed.

@googlebot
Copy link

CLAs look good, thanks!

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #833   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files          68       68           
  Lines        3144     3144           
  Branches      387      387           
=======================================
  Hits         3142     3142           
  Misses          2        2

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 184897b...c1933db. Read the comment docs.

@Garbee
Copy link
Contributor

Garbee commented Jun 15, 2017

This runs contrary to the guidelines,

We should get input from the MD team on this then. As, what is currently provided is exactly what is specified here. The default's shouldn't go against that.

@unascribed
Copy link
Author

unascribed commented Jun 15, 2017

I'd tend to agree, but I was (somewhat indirectly) asked to PR this by @traviskaufman on #819.

@Garbee
Copy link
Contributor

Garbee commented Jun 15, 2017

I am not asking for a merge block. Simply a delay to get feedback from the design team on if this is appropriate. As what change is made by this PR is specifically against my entire understanding of the button component from when I made the original base for MCW.

If I were to comment on the issue with that, it could easily get lost/looked over. Best keep the discussions targeted on the issue/PR in question rather than spreading conversation out.

@amsheehan
Copy link
Contributor

I won’t be available until Monday. @YiranM or @lynnjepsen, could you facilitate this?

@lynnmercier
Copy link
Contributor

I apologize for not replying sooner, but I've found some time to look into this issue. I've written a longer explanation on the original issue and created a new PR to solve the original issue.

@amsheehan
Copy link
Contributor

Closing, as @lynnjepsen seems to have confirmed what @Garbee and the spec says with design. Thanks again @unascribed, we really appreciate the effort, but in this case I think we're seeing a case that should be handled on an implementation level as regards color for mdc-button's on a dark theme.

@amsheehan amsheehan closed this Jun 23, 2017
@unascribed unascribed deleted the patch-1 branch June 23, 2017 17:50
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.

6 participants