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: re-enable color prop for the badge component #648

Merged
merged 3 commits into from
Dec 5, 2017

Conversation

machour
Copy link
Member

@machour machour commented Nov 25, 2017

Question Response
Version? master
Devices tested? iPhone 7, Android Nexus 5
Bug fix? yes
New feature? no
Includes tests? no
All Tests pass? yes
Related ticket? #581

Screenshots

Before After
screen shot 2017-11-25 at 2 41 37 pmscreen shot 2017-11-25 at 2 46 08 pm screen shot 2017-11-25 at 2 48 19 pm screen shot 2017-11-25 at 2 48 12 pm

Description

#535 seems to have lost the color prop for the badge text, making it always black (the default).
This fixed the style be reading from that prop again when it's available.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 39.784% when pulling 7998ee8 on machour:fix-notifications-color into 7fc0681 on gitpoint:master.

@@ -25,6 +25,7 @@ const BadgeContainer = styled.View`
const BadgeText = styled.Text`
${{ ...fonts.fontPrimaryBold }};
background-color: transparent;
${({ color }) => `color: ${color || colors.black};`};
Copy link
Member

@chinesedfan chinesedfan Nov 25, 2017

Choose a reason for hiding this comment

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

It reminds me a question that, what's the differences between this way with color: ${({color}) => (color || color.black)};?

Copy link
Member

@chinesedfan chinesedfan Dec 3, 2017

Choose a reason for hiding this comment

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

Both styles are OK. But the one I mentioned is more similar with styled-components official documents.

Copy link
Member Author

Choose a reason for hiding this comment

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

@chinesedfan just had the time to test, it's exactly the same.
I switched to the syntax you proposed as it's clearer.

I guess the difference between the two syntaxes, is that with the one I originally used, you can choose to output no style at all based on tests, which is not possible with the other one:

${({ foo }) => foo ? `color: '#FFCC00'` : ''};

You could also output more styles in the condition

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0009%) to 39.861% when pulling 3f9cab5 on machour:fix-notifications-color into 7fc0681 on gitpoint:master.

@chinesedfan chinesedfan merged commit 14532ce into gitpoint:master Dec 5, 2017
@machour machour deleted the fix-notifications-color branch December 5, 2017 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants