Skip to content

[RaisedButton] Add buttonStyle prop#4256

Closed
c089 wants to merge 1 commit intomui:masterfrom
c089:pass-through-button-style
Closed

[RaisedButton] Add buttonStyle prop#4256
c089 wants to merge 1 commit intomui:masterfrom
c089:pass-through-button-style

Conversation

@c089
Copy link
Copy Markdown

@c089 c089 commented May 13, 2016

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Closes #4345

@c089 c089 changed the title Pass through style to EnhancedButton [RaisedButton] Pass through style to EnhancedButton May 13, 2016
@c089 c089 force-pushed the pass-through-button-style branch from 212a864 to fff9445 Compare May 13, 2016 14:37
transition: transitions.easeOut(),
},
button: {
button: Object.assign({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should go in render()

@nathanmarks
Copy link
Copy Markdown
Contributor

nathanmarks commented May 15, 2016

@c089 style is already being put onto the root node, this would mean we're passing that prop to multiple nodes...

@oliviertassinari This is kind of like that other one. Personally, I think that we have an issue similar to this with most components that have Paper at the root node. Honestly, I wouldn't even use a Paper component just for a shadow so we could avoid this sort of issue, but that's another discussion 😄

@c089
Copy link
Copy Markdown
Author

c089 commented May 17, 2016

@nathanmarks

@c089 style is already being put onto the root node, this would mean we're passing that prop to multiple nodes...

True, that would be confusing. Would you accept this PR if it added a buttonStyle prop instead?

@oliviertassinari This is kind of like that other one. Personally, I think that we have an issue similar to this with most components that have Paper at the root node. Honestly, I wouldn't even use a Paper component just for a shadow so we could avoid this sort of issue, but that's another discussion 😄

Agreed, I was surprised by this here as well...

@kidwm
Copy link
Copy Markdown

kidwm commented May 24, 2016

vote for buttonStyle instead.

@c089 c089 force-pushed the pass-through-button-style branch from fff9445 to b696210 Compare May 25, 2016 07:53
@c089 c089 changed the title [RaisedButton] Pass through style to EnhancedButton [RaisedButton] Add buttonStyle prop May 25, 2016
@c089
Copy link
Copy Markdown
Author

c089 commented May 25, 2016

@kidwm @nathanmarks I updated the PR to add a buttonStyle prop instead.

I noticed this is inconsistent with the API of FlatButton though, which does use style for it's EnhancedButton, so that might be a separate change to consider for the next release that breaks APIs.

@c089 c089 force-pushed the pass-through-button-style branch from b696210 to 7599b38 Compare May 25, 2016 07:58
@nathanmarks
Copy link
Copy Markdown
Contributor

@mbrookes How do we make this consistent when one button has a completely different root node? ahhh! 🔨 🔥

@mbrookes
Copy link
Copy Markdown
Member

@nathanmarks With a single Button component? 😄

@oliviertassinari
Copy link
Copy Markdown
Member

@c089 Thanks for this PR. We have added a buttonStyle property.

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