-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(pirateship): fixes prods text and shop all cats to dark gray #130
Conversation
dfrho
commented
Aug 3, 2018
- Featured Products and Shop All Categories text color set to palette.secondary === grays.eight
- Same with brand text (ie. "Sony")
@@ -112,8 +113,8 @@ const ShopStyle = StyleSheet.create({ | |||
|
|||
export interface ShopProps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no additional props, this can be a type.
export type ShopProps = ScreenProps & Pick<........>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add to punch list
@dfrho - you don't need to implement pav's suggestion because you didn't modify the types, but you'll have to fix this conflict. Let me know if you need any assistance. |
Added navigator type to ScreenWrapper and settled merge conflicts. |
export interface PSScreenWrapperProps { | ||
style?: StyleProp<ViewStyle>; | ||
|
||
hideGlobalBanner?: boolean; | ||
overrideGlobalBanner?: PSGlobalBannerSlotItem; | ||
|
||
needInSafeArea?: boolean; | ||
navigator?: Navigator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary because we're defining navigator on line 46. If you're getting a tsc error it's probably coming from somewhere else.
just pushed a cleaner version removing the extra navigator props on interface PSScreenWrapperProps. |
@dfrho - conflicts :( I'd recommend copying your work into a new branch and submitting a new PR. |
@bweissbart on it, thanks Brett |