Skip to content

refactor(badge): refactor badge component to use styled-components#535

Merged
alejandronanez merged 6 commits intogitpoint:masterfrom
hirejohnsalcedo:refactor/532-badge-component
Oct 22, 2017
Merged

refactor(badge): refactor badge component to use styled-components#535
alejandronanez merged 6 commits intogitpoint:masterfrom
hirejohnsalcedo:refactor/532-badge-component

Conversation

@hirejohnsalcedo
Copy link
Copy Markdown
Contributor

@hirejohnsalcedo hirejohnsalcedo commented Oct 21, 2017

Related to #532

@andrewda andrewda changed the title refactor($component): refactor badge component to use styled-components refactor(badge): refactor badge component to use styled-components Oct 21, 2017
Comment thread src/components/badge.component.js Outdated
@@ -1,5 +1,5 @@
import React from 'react';
import { StyleSheet, View, Text } from 'react-native';
import styled from 'styled-components';
Copy link
Copy Markdown
Member

@chinesedfan chinesedfan Oct 21, 2017

Choose a reason for hiding this comment

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

Please import from 'styled-components/native'.

@hirejohnsalcedo
Copy link
Copy Markdown
Contributor Author

@chinesedfan 🎉

Copy link
Copy Markdown
Member

@alejandronanez alejandronanez left a comment

Choose a reason for hiding this comment

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

Can you address these two changes before merging?
Thanks @jpls93 and welcome to Gitpoint!

height: 18;
border-color: ${colors.alabaster};
border-width: 1;
${({ backgroundColor }) => `background-color: ${backgroundColor};`};
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.

I think you can do background-color: ${backgroundColor} instead,

const BadgeText = styled.Text`
${{ ...fonts.fontPrimaryBold }};
background-color: 'transparent';
${({ largeText }) =>
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.

Same goes for this font-size: ${largeText ? normalize(9.5) : normalize(7)}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @alejandronanez , might not work since unlike border-color: ${colors.alabaster};, colors is a file-wide variable whereas backgroundColor and largeText are props passed down to the styled components. https://www.styled-components.com/docs/basics#adapting-based-on-props

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@alejandronanez , let me know if I can help you with anything else

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.

Oh yeah, you're right. I missed that, sorry.
Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the usual way would be this though:

...
    font-size: ${({largeText}) => largeText ? normalize(9.5) : normalize(7)};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that would be more concise. I guess I just made the code style more consistent by making it parallel to how I wrote BadgeContainer's backgroundColor which doesn't have a default value. Since writing something without a default value that way would make it background-color: undefined which, without tree-shaking, would yield dead code.

@alejandronanez alejandronanez merged commit 0d2f2e9 into gitpoint:master Oct 22, 2017
@hirejohnsalcedo hirejohnsalcedo deleted the refactor/532-badge-component branch October 22, 2017 16:59
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.

5 participants