refactor: NotificationListItem to used styled-components#610
refactor: NotificationListItem to used styled-components#610machour merged 3 commits intogitpoint:masterfrom
Conversation
chinesedfan
left a comment
There was a problem hiding this comment.
@LeoCp Welcome to GitPoint!
| }); | ||
| const NotificationListItemContainer = styled.View` | ||
| border-bottom-color: ${colors.greyLight}; | ||
| border-bottom-width: 1px; |
There was a problem hiding this comment.
@LeoCp Can you omit units unless it is a shorthand property?
|
|
||
| const Title = styled.Text` | ||
| color: ${colors.black}; | ||
| ${{ ...fonts.fontPrimary }}; |
There was a problem hiding this comment.
Please try to use font-family: ${styledFonts.fontPrimary}, while styledFonts is imported from config.
| type: 'octicon', | ||
| })``; | ||
|
|
||
| const CheckIcon = styled(IconStyled).attrs({ name: 'check' })``; |
There was a problem hiding this comment.
Can the empty block be removed?
| getIconName = type => { | ||
| switch (type) { | ||
| case 'commit': | ||
| case 'Commit': |
There was a problem hiding this comment.
Is this a case bug that you find out?
There was a problem hiding this comment.
yes, the type was wrong.
There was a problem hiding this comment.
optional: I think it will be good to transform type to lowercase and to the check, this way we cover ourselves from any future change.
What do you think?
There was a problem hiding this comment.
Agree, but should be done in another PR.
| const TitleComponent = this.getComponentType(); | ||
| const tag = this.getComponentType(); | ||
| const iconName = this.getIconName(notification.subject.type); | ||
| const titleComponentProps = |
There was a problem hiding this comment.
Hey @LeoCp welcome!
Can you please move this into a class method and add proper tests to it? I don't feel comfortable with all this logic under render() { ... }
|
@alejandronanez could you verify that the latest changes are okay with you and merge this baby in? ❤️ |
|
Validated on gitter with Alejandro 🕺 |
Description
Migrate NotificationListItem component to styled-components (#532)