Fix/design review #52
Conversation
| </Headline> | ||
| <Body style={styles.content}>{props.content}</Body> | ||
| <View style={styles.actions}> | ||
| {props.onDismiss && props.dismissButtonLabel && ( |
There was a problem hiding this comment.
The onDismiss props is mandatory so I think checking only dismissButtonLabel is enough here
There was a problem hiding this comment.
Note to myself: this optional "dismiss" button does not exists in the design system.
We need to ask the UX to update it accordingly.
There was a problem hiding this comment.
I found only one case, where the dialog appears with single button, so that's why one of the buttons are conditional here
There was a problem hiding this comment.
Thanks for the info, I will show it to the UX :)
src/components/buttons/Button.tsx
Outdated
| } else { | ||
| return ( | ||
| <BaseButton style={style} labelStyle={labelStyle} onPress={onClick} testID={testID}> | ||
| <BaseButton |
There was a problem hiding this comment.
Thinking about it I think it's not a good idea to fallback to the default react-native-paper's Button component if no mode props has been provided.
I cannot tell what it looks like quickly because it is not in the design system.
Would you mind to provide a default value to the mode props instead ? Like filled ?
| marginHorizontal: 0, | ||
| ...labelStyle, | ||
| const getbackgroundColor = (): ViewStyle['backgroundColor'] => { | ||
| switch (status) { |
There was a problem hiding this comment.
Could you move this function in it's own file (like utils), so that we can reuse it in the 3 button variants ?
Something like getColor(status) -> string so it could be reused without being concerned wether we want to set the background color, the text color or the border color ?
| <Stack.Navigator initialRouteName="Home"> | ||
| <Stack.Navigator | ||
| screenOptions={{ | ||
| contentStyle: { backgroundColor: '#FFF' }, |
There was a problem hiding this comment.
I think i tried to use theme there, but it returns an error as i try to use it outside ThemeProvider
There was a problem hiding this comment.
We definitely should use the ThemeProvider then !
No description provided.