Skip to content

added flatbutton background color property#1561

Merged
shaurya947 merged 1 commit intomui:masterfrom
yongxu:flat-button-bgcolor
Sep 30, 2015
Merged

added flatbutton background color property#1561
shaurya947 merged 1 commit intomui:masterfrom
yongxu:flat-button-bgcolor

Conversation

@yongxu
Copy link
Copy Markdown
Contributor

@yongxu yongxu commented Aug 31, 2015

added backgroundColor property.
The previous code only has hovered background color property

@shaurya947
Copy link
Copy Markdown
Contributor

shaurya947 commented Sep 29, 2015

Thanks @yongxu, could you rebase?

@yongxu yongxu force-pushed the flat-button-bgcolor branch from 5dfcc00 to f2fa4db Compare September 30, 2015 07:04
@yongxu
Copy link
Copy Markdown
Contributor Author

yongxu commented Sep 30, 2015

Thanks @shaurya947, I have rebased it with the latest master branch!
Looks like the Travis Ci fail is caused by either npm or node 4.0, the rebase should work fine.

@oliviertassinari
Copy link
Copy Markdown
Member

oliviertassinari commented Sep 30, 2015 

Message:
    "contextProps" is not defined.

shaurya947
shaurya947 reviewed Sep 30, 2015
View reviewed changes
src/flat-button.jsx Outdated
Copy link
Copy Markdown
Contributor

@shaurya947 shaurya947 Sep 30, 2015

Choose a reason for hiding this comment

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

contextProps is no longer used. You probably need to change this to:

const buttonBackgroundColor = backgroundColor || buttonColor;

Also, I was thinking that it would be nice to add the backgroundColor property to the flatButton key inside theme-manager too. What do you think? That way, we can check in this order of precedence:

backgroundColor prop > backgroundColor theme value > buttonColor

cc @oliviertassinari

@oliviertassinari
Copy link
Copy Markdown
Member

oliviertassinari commented Sep 30, 2015

I agree with you @shaurya947.

@yongxu
Copy link
Copy Markdown
Contributor Author

yongxu commented Sep 30, 2015

@shaurya947 the buttonColor actually indicates the background color, so it is already in the theme manager. The text color is defined here:

    const defaultColor = disabled ? disabledTextColor :
      primary ? primaryTextColor :
      secondary ? secondaryTextColor :
      textColor;

I do agree with you on adding backgroundColor, in that case we only need to change the name from buttonColor to backgroundColor. Personally I don't think it is necessary and it may break some code somewhere else.

@yongxu yongxu force-pushed the flat-button-bgcolor branch from f2fa4db to 43fe9ef Compare September 30, 2015 18:14
@yongxu
Copy link
Copy Markdown
Contributor Author

yongxu commented Sep 30, 2015

@shaurya947 the problem should have been fixed. sorry didn't test it after rebase.

@shaurya947
Copy link
Copy Markdown
Contributor

shaurya947 commented Sep 30, 2015

Gotcha @yongxu. I'll go ahead and merge this. Thanks!

shaurya947 added a commit that referenced this pull request Sep 30, 2015
@shaurya947
Merge pull request #1561 from yongxu/flat-button-bgcolor
7d15009 
added flatbutton background color property
@shaurya947 shaurya947 merged commit 7d15009 into mui:master Sep 30, 2015
@zannager zannager added the scope: button Changes related to the button. label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

scope: button Changes related to the button.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yongxu @shaurya947 @oliviertassinari @zannager