refactor: members to styled components#606
Conversation
| width: 15; | ||
| height: ${avatarSize}; | ||
| ${props => props.gradientPosition}; | ||
| `; |
There was a problem hiding this comment.
Some of these have new lines between them, others do not. Is there a pattern you're following or can we clean this up a little?
| `; | ||
|
|
||
| const SectionTitle = styled.Text` | ||
| color: ${props => (props.isTitleSmall ? colors.primaryDark : colors.black)}; |
There was a problem hiding this comment.
@shmesa22 Maybe brackets should be removed to keep consistent.
There was a problem hiding this comment.
@chinesedfan They were added by prettier
machour
left a comment
There was a problem hiding this comment.
hey again @shmesa22 :)
left you some comments on this PR as well so that we can enhance our usage of styled-components. Some of the requested changes will have to wait for #619 which should be merged pretty soon.
Don't hesitate if you need any help with my feedback
| const NoMembersList = styled(StyledList)` | ||
| margin-top: 0; | ||
| `; | ||
|
|
There was a problem hiding this comment.
Instead of creating a new component, once #619 is merged you can do this as follow:
const NoMembersList = styled(List).attrs({
containerStyle: {
margin-top: 0;
}
})``;| padding-left: 15; | ||
| padding-right: 15; | ||
| `; | ||
|
|
src/components/members.component.js
Outdated
| <Text style={smallTitle ? styles.sectionTitleSmall : styles.sectionTitle}> | ||
| {title} | ||
| </Text> | ||
| <Container style={containerStyle}> |
There was a problem hiding this comment.
Scanned the code to see where containerStyle is passed, it's only used in <IssueDescription> as follows:
<MembersList
title={translate('issue.main.assignees', locale)}
members={issue.assignees}
containerStyle={{ marginTop: 0, paddingTop: 0, paddingLeft: 0 }}
smallTitle
navigation={navigation}
/>
Could you introduce a new boolean prop, like "noMargin" and get rid of containerStyle?
The Container styled component should be updated to read this new props and add the extra css declarations instead. IssueDescription will need to be updated and be part of this PR of course.
Let me know if this is unclear to you, I'll be happy to help
src/components/members.component.js
Outdated
| styles.avatarContainer, | ||
| { marginRight: index < members.length - 1 ? 5 : 0 }, | ||
| ]} | ||
| style={{ marginRight: index < members.length - 1 ? 5 : 0 }} |
There was a problem hiding this comment.
underlayColor & style should be generated by styled-components instead
| end={{ x: 1, y: 0.5 }} | ||
| <ScrollGradient | ||
| gradientPosition={{ right: 0 }} | ||
| gradientColors={['rgba(255, 255, 255, 0)', 'white']} |
There was a problem hiding this comment.
we could omit the gradient prefix from prop names, as the component name is ScrollGradient ;)
|
@shmesa22, #619 is in, feel free to resume you work at any time (no pressure ❤️ ) |
|
Sorry I went on unexpected 'vacations'. I'll finish all my pending issues ASAP |
|
Hey @shmesa22, what's the status on this one ? 🤔 |
|
Hi @machour, I'll finish my pending issues this weekend |
Changed from styledFonts to fonts and changed removed containerStyle and added noMargin boolean prop to add extra css declarations if true
|
Perfect 👌thank you so much! |
Screenshots
Description
Migrate Members component to styled-components (#532)