Skip to content

[FlatButton] Fix Icon color prop issue #4160

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 6, 2016
Merged

[FlatButton] Fix Icon color prop issue #4160

merged 1 commit into from
May 6, 2016

Conversation

@tintin1343
Copy link
Contributor

@tintin1343 tintin1343 commented May 4, 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).

Resolves: #4144

color prop of icon was not being passed to the icon used within a flat-button

Replaced color: mergedRootStyles.color, with color: icon.props.color, and that fixed it.

    <FlatButton
      backgroundColor="#a4c639"
      hoverColor="#8AA62F"
      icon={<ActionAndroid color={fullWhite} />}
      style={style}
    />

screen shot 2016-05-04 at 2 22 38 pm

mbrookes
mbrookes reviewed May 4, 2016
View reviewed changes
src/FlatButton/FlatButton.js Outdated
if (icon) {
iconCloned = React.cloneElement(icon, {
color: mergedRootStyles.color,
color: icon.props.color,
Copy link
Member

@mbrookes mbrookes May 4, 2016

Choose a reason for hiding this comment

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

icon.props.color || mergedRootStyles.color ?

Copy link
Contributor Author

@tintin1343 tintin1343 May 4, 2016

Choose a reason for hiding this comment

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

Oh yeah..shoot. I missed the case when there is not color prop passed.

Thanks @mbrookes for pointing it out.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 4, 2016

@tintin1343 That looks good. Do you want to write a test for this specific case?

@tintin1343
Copy link
Contributor Author

tintin1343 commented May 5, 2016 •
edited
Loading

@oliviertassinari : Yeah sure. Let me give it a try.

@tintin1343
Copy link
Contributor Author

tintin1343 commented May 5, 2016

@oliviertassinari : Done.

oliviertassinari
oliviertassinari reviewed May 5, 2016
View reviewed changes
src/FlatButton/FlatButton.spec.js Outdated
/>
);
const icon = wrapper.children().at(0);
assert.equal(icon.prop('color'), 'white', 'icon should have same color as that of color prop');
Copy link
Member

@oliviertassinari oliviertassinari May 5, 2016

Choose a reason for hiding this comment

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

strictEqual

Copy link
Contributor Author

@tintin1343 tintin1343 May 5, 2016

Choose a reason for hiding this comment

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

Done.

oliviertassinari
oliviertassinari reviewed May 5, 2016
View reviewed changes
src/FlatButton/FlatButton.spec.js Outdated
<FlatButton
backgroundColor="#a4c639"
hoverColor="#8AA62F"
icon={<ActionAndroid color={'white'} />}
Copy link
Member

@oliviertassinari oliviertassinari May 5, 2016

Choose a reason for hiding this comment

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

I would put 'white' in a variable so the link is explicit in the test.

Copy link
Contributor Author

@tintin1343 tintin1343 May 5, 2016

Choose a reason for hiding this comment

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

Alright. Thats a good idea.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 5, 2016

@tintin1343 That looks good, nice 👍.

@oliviertassinari oliviertassinari added this to the 0.15.1 Release milestone May 5, 2016
@tintin1343
Copy link
Contributor Author

tintin1343 commented May 5, 2016 •
edited
Loading

Rebase Done.

oliviertassinari
oliviertassinari reviewed May 6, 2016
View reviewed changes
src/FlatButton/FlatButton.spec.js Outdated
});

it('colors the icon with the passed color in prop', () => {
let color = 'white';
Copy link
Member

@oliviertassinari oliviertassinari May 6, 2016

Choose a reason for hiding this comment

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

Shouldn't it be const?

Copy link
Contributor Author

@tintin1343 tintin1343 May 6, 2016

Choose a reason for hiding this comment

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

You are right. I changed it to const

@mbrookes mbrookes merged commit 8d68fb9 into mui:master May 6, 2016
@oliviertassinari
Copy link
Member

oliviertassinari commented May 6, 2016

@tintin1343 Nice!

@FaideWW FaideWW mentioned this pull request Jun 20, 2016
Closed
@zannager zannager added component: Icon The React component. scope: button Changes related to the button. labels Mar 14, 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

component: Icon The React component. scope: button Changes related to the button.

Projects

None yet

Milestone

v0.15.1

Development

Successfully merging this pull request may close these issues.

[FlatButton] icon props are not passed through

4 participants

@tintin1343 @oliviertassinari @mbrookes @zannager