Skip to content

[Inspiration screens]: Product Page & Twitter screens #1466

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

Merged
merged 7 commits into from
Aug 17, 2021
Merged

Conversation

mariaborovyk
Copy link

@mariaborovyk mariaborovyk commented Aug 10, 2021

Description

Created Demo Screens in Inspiration Screens section

Changelog

demo/src/index.js
demo/src/screens/MenuStructure.js
demo/src/screens/realExamples/ProductPage/index.tsx - new one
demo/src/screens/realExamples/Twitter/index.tsx - new one
demo/src/screens/realExamples/index.js

@mariaborovyk mariaborovyk requested a review from ethanshar August 11, 2021 07:58
@mariaborovyk mariaborovyk self-assigned this Aug 11, 2021
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Wrote a few comments

Comment on lines 77 to 96
<View marginV-s2 paddingV-s2 style={{borderColor: Colors.dark70, borderBottomWidth: 1}}>
<Text dark30>Color</Text>
<TouchableOpacity row centerV spread onPress={()=> this.setState({isColor: true})}>
<View row centerV>
<View style={{width: 14, height: 14, borderRadius: 12, marginRight: 6}} backgroundColor={colorOption.color}/>
<Text dark10 text70>{colorOption.name}</Text>
</View>
<Image source={require('../../../assets/icons/chevronDown.png')}/>
</TouchableOpacity>
</View>
<ActionSheet
title={'Select a Color'}
options={[
{label: colorOptions.red.name, onPress: () => this.pickOption(colorOptions.red)},
{label: colorOptions.green.name, onPress: () => this.pickOption(colorOptions.green)},
{label: colorOptions.blue.name, onPress: () => this.pickOption(colorOptions.blue)},
]}
visible={isColor}
onDismiss={()=> this.setState({isColor: false})}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the Picker component?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that Picker doesn't have ActionSheet, but seems like it has in custom options

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, technically you achieve the same behavior with custom item render.

Comment on lines 98 to 116
<View marginV-s2 paddingV-s2 style={{borderColor: Colors.dark70, borderBottomWidth: 1}}>
<Text dark30>Size</Text>
<TouchableOpacity row centerV spread onPress={()=> this.setState({isSize: true})}>
<View row centerV>
<Text dark10 text70>{sizeOption.name}</Text>
</View>
<Image source={require('../../../assets/icons/chevronDown.png')}/>
</TouchableOpacity>
</View>
<ActionSheet
title={'Select a Size'}
options={[
{label: sizeOptions.s.name, onPress: () => this.pickOption(sizeOptions.s)},
{label: sizeOptions.m.name, onPress: () => this.pickOption(sizeOptions.m)},
{label: sizeOptions.l.name, onPress: () => this.pickOption(sizeOptions.l)},
]}
visible={isSize}
onDismiss={()=> this.setState({isSize: false})}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, consider using the Picker component

@ethanshar
Copy link
Collaborator

@maeschli Let me know if you still have time to fix my comments.
I can always merge and we can handle it later on in a different PR

@mariaborovyk mariaborovyk changed the title [Inspiration screens]: Product Page Screen [Inspiration screens]: Product Page & Twitter screens Aug 15, 2021
@ethanshar ethanshar enabled auto-merge (squash) August 17, 2021 08:42
@ethanshar ethanshar merged commit 788410c into master Aug 17, 2021
@ethanshar ethanshar deleted the inspo-screens branch April 18, 2022 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants