refactor: use styled-components in comment-list-item#557
refactor: use styled-components in comment-list-item#557MrLoh wants to merge 2 commits intogitpoint:masterfrom MrLoh:styled-components-comment-list-item-component
Conversation
|
the |
|
@MrLoh I'll try to fix that. |
|
@MrLoh try adding yourself now |
|
cool, thant worked. I rebased of master and added myself. Should be ready to merge |
| "name": "Tobias Lohse", | ||
| "avatar_url": "https://avatars0.githubusercontent.com/u/1285032?v=4", | ||
| "profile": "http://MrLoh.se", | ||
| "contributions": [] |
There was a problem hiding this comment.
🎉 You contributed codes in fact.
|
|
||
| const Header = styled.View` | ||
| flex-direction: row; | ||
| margin-left: 10px; |
There was a problem hiding this comment.
Nice catch for those shorthands properties. But shall we always add units for properties? (cc @alejandronanez)
There was a problem hiding this comment.
I don’t know. It’s up to you. I never use units for 0 in css.
There was a problem hiding this comment.
I think we should stick to unitless, that's what we've been doing in other PRs
| `; | ||
|
|
||
| const LinkDescription = styled.Text` | ||
| ${{ ...fonts.fontPrimaryBold }}; |
There was a problem hiding this comment.
Can we just write as ${fonts.fontPrimaryBold}?
There was a problem hiding this comment.
I don’t kno. I’m not sure how this actually works. Just copied it from the other already concerted components. I handle fonts differently in my apps.
There was a problem hiding this comment.
This should be:
font-family: ${styledFonts.fontPrimaryBold}
EDIT: Actually, let's keep it like you did, I think it would be better to get rid of styledFonts. I'll open a PR for this.
| `; | ||
|
|
||
| const CommentTextNone = styled.Text` | ||
| ${{ ...fonts.fontPrimary }}; |
alejandronanez
left a comment
There was a problem hiding this comment.
Can you please remove the units (px). I recall not using units in other PRs related to styled-components.
|
|
||
| const Header = styled.View` | ||
| flex-direction: row; | ||
| margin-left: 10px; |
There was a problem hiding this comment.
I think we should stick to unitless, that's what we've been doing in other PRs
|
@alejandronanez I don't think it's a good idea to go unitless, as it is against the styled-component docs and will break percentage units. I'm also a bit confused as to who is in charge of reviewing and merging pull requests as two other of my pull-requests with the same style issues where accepted and merged allready. #559 #558 |
|
@MrLoh Yes, styled-component requires units in sometimes, but it is only for shorthands properties. And sorry about previous PR reviews are inconsistent. |
| `; | ||
|
|
||
| const LinkDescription = styled.Text` | ||
| ${{ ...fonts.fontPrimaryBold }}; |
There was a problem hiding this comment.
This should be:
font-family: ${styledFonts.fontPrimaryBold}
EDIT: Actually, let's keep it like you did, I think it would be better to get rid of styledFonts. I'll open a PR for this.
|
hey @MrLoh, I've got a PR pending to switch all styled-components to use the also, as @chinesedfan & @alejandronanez explained, we went unit-less for styled-components, it would be great if you could update your PR accordingly. let us know if you need any help, let's get this PR merged 🎉 🎉 |
|
Launched by #723. |
related to #532