-
Notifications
You must be signed in to change notification settings - Fork 788
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
refactor(styles): Migrate IssueListItem to Styled Components #578
base: master
Are you sure you want to change the base?
Conversation
Migrate Stylesheet styles to styled components. gitpoint#532
align-items: flex-start; | ||
justify-content: center; | ||
padding-right: 10; | ||
padding: 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorthands properties should have units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
paddingVertical: 5, | ||
borderBottomWidth: 1, | ||
borderBottomColor: colors.greyLight, | ||
}, | ||
closedIssue: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be removed, because nowhere uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closeIssue style is used in Line 70.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I forget to expand those collapsed codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a screenshot of the work you've done?
LGTM and Thanks!
@@ -99,10 +103,10 @@ export const IssueListItem = ({ type, issue, navigation, locale }: Props) => ( | |||
titleStyle={styles.title} | |||
subtitleStyle={styles.subtitle} | |||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once #619 is merged, this <ListItem>
can be converted as a styled component using this:
const Issue = styled(ListItem).attrs({
containerStyle: {
// old style attributes
},
titleStyle: {
// old style attributes
},
subtitleStyle: {
// old style attributes
}
})``;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#619 is merged, you should now be able to convert the react-native-elements components.
@@ -99,10 +103,10 @@ export const IssueListItem = ({ type, issue, navigation, locale }: Props) => ( | |||
titleStyle={styles.title} | |||
subtitleStyle={styles.subtitle} | |||
/> | |||
<View style={styles.commentsContainer}> | |||
<CommentsContainer> | |||
<Icon name="comment" type="octicon" size={18} color={colors.grey} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should define a new styled component here (same way as ListItem), and have all this static props in the styled component.
The JSX code would then simply be something like: <CommentIcon />
What do you think of this? 🤔
Hi @josenaranjo, friendly ping, are you still up for this one? 🙏 |
I left some StyleSheet styles without migrating because those styles are so specific about a particular behavior that 1. I'm not sure they are going to display the component as expected and 2. It has more meaning as it is. Let me know what you think.
#532