Skip to content

Conversation

@ffxsam
Copy link
Contributor

@ffxsam ffxsam commented Feb 28, 2016

Strange. This is documented, though it was apparently ignored in the code.

@oliviertassinari
Copy link
Member

Looks like it the same for the labelColor property.

} else if (secondary) {
backgroundColor = raisedButton.secondaryColor;
labelColor = raisedButton.secondaryTextColor;
} else if (props.backgroundColor) {
Copy link
Member

Choose a reason for hiding this comment

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

What about using this logic?

} else {
  if (props.backgroundColor) {
    backgroundColor = props.backgroundColor || raisedButton.secondaryColor;
  }
  if (props.labelColor) {
    labelColor = props.labelColor || raisedButton.secondaryTextColor;
  }
}

@oliviertassinari oliviertassinari changed the title backgroundColor prop was previously ignored [RaisedButton] backgroundColor prop was previously ignored Feb 28, 2016
@ffxsam
Copy link
Contributor Author

ffxsam commented Feb 28, 2016

Well, you're already setting your defaults here:

  let backgroundColor = raisedButton.color;
  let labelColor = raisedButton.textColor;

So in the proposed else block, why set backgroundColor to raisedButton.secondaryColor if props.backgroundColor is undefined? I would recommend either:

  1. Get rid of these two lines and implement your suggestion.
  2. Leave those two lines in, and I'd modify my code to be:
  } else {
    if (props.backgroundColor) {
      backgroundColor = props.backgroundColor;
    }
    if (props.labelColor) {
      labelColor = props.labelColor;
    }
  }

@ffxsam
Copy link
Contributor Author

ffxsam commented Feb 28, 2016

Actually, to change option 1 above.. I wouldn't get rid of the lines, just change them to:

let backgroundColor, labelColor;

I'm a proponent of declaring variables at the top of scopes so they're easy to find.

@ffxsam
Copy link
Contributor Author

ffxsam commented Feb 28, 2016

Hmm, this might be a mistake actually? There are no such properties on raisedButton in src/styles/getMuiTheme.js

@ffxsam
Copy link
Contributor Author

ffxsam commented Feb 28, 2016

I went with option 2 above, because it introduces the least amount of changes. If we had gone with option 1, buttons without primary or secondary set would've been a breaking change.

@oliviertassinari
Copy link
Member

That looks good to me 👍.

@alitaheri
Copy link
Collaborator

This is great, thanks 👍 👍

alitaheri added a commit that referenced this pull request Feb 28, 2016
[RaisedButton] backgroundColor prop was previously ignored
@alitaheri alitaheri merged commit f89a924 into mui:master Feb 28, 2016
@ffxsam ffxsam deleted the raised-button-color-fixes branch February 28, 2016 20:29
mbradleyis pushed a commit to staticinstance/material-ui that referenced this pull request Mar 7, 2016
[RaisedButton] backgroundColor prop was previously ignored
@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

Labels

scope: button Changes related to the button.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants