Refactor StateBadge component#715
Conversation
| @@ -1,5 +1,5 @@ | |||
| import React from 'react'; | |||
| import { Text, View, StyleSheet } from 'react-native'; | |||
| import styled from 'styled-components/native'; | |||
There was a problem hiding this comment.
no need to use '/native' anymore, just use this:
import styled from 'styled-components';
| issueStateColor = colors.red; | ||
| break; | ||
| default: | ||
| issueStateColor = colors.gray; |
There was a problem hiding this comment.
could you do something like this instead for a better readability?
const stateColor = {
merged: colors.purple,
open: colors.green,
closed: colors.red,
};
const issueStateColor = stateColor[issueState] ? stateColor[issueState] : colors.gray;| import { colors, fonts, normalize } from 'config'; | ||
| import { v3 } from 'api'; | ||
|
|
||
| const styles = StyleSheet.create({ |
There was a problem hiding this comment.
It was used just for the Badge styles so I removed it. I don't think is needed here, also styled-components are already imported.
Also the styles defined for the Badge don't change the UI so they are useless for what I can saw.
| <View style={[styles.badge, style, issueStyle]}> | ||
| <Text style={styles.text}>{issueText}</Text> | ||
| </View> | ||
| <Badge color={issueStateColor} style={style}> |
There was a problem hiding this comment.
You are not passing style anymore from issue-description.component.js, but you're still using the prop here .. Could you give it a second look?
There was a problem hiding this comment.
Yeah I thought to get completely rid of the style prop but it could still be useful in case there is a need to override something from a parent component without modifying again this one instead.
But it's up for discussion, don't know if it is really needed or not so I left it there 😆
There was a problem hiding this comment.
Prop style is also used in repository-section-title.component.js, which is a useless component. I remembered @housseindjirdeh confirmed that in somewhere, but I forgot the detail issue/pr number. I will collect feedbacks in #725.
There was a problem hiding this comment.
it was on Gitter, I added the relevant screenshot in the issue you opened.
|
Don't really know how the tests work yet 😆 I'll get through them tomorrow 👍 |
|
No worries mate! We use jest for testing, and everything is located under or you can run a single test like this (which is waaaayyyy faster when fixing something 😄) : happy debugging 🕺 |
|
What should I do to augment the coverage? |
Don't worry about coverage for now. Dismissing the review so I remember to re-check this PR (and especially your comments)
chinesedfan
left a comment
There was a problem hiding this comment.
Tested in iPhone 6 simulator. LGTM!
housseindjirdeh
left a comment
There was a problem hiding this comment.
Great stuff, thank you @rkpasia 🙏
Screenshots
Description
This PR is related with the update of the StateBadge component. Now it uses styled-components as a driver to style the user interface.
I kept the possibility to override the styles of the StateBadge through the
styleprop. The object that it accept it's a StyleSheet object.Also I removed from issue-description component the object
styles. It was only related to the StateBadge component but the styles declared didn't influenced the UI placement of the latter.