Skip to content

refactor(ui): update language selectionscreen #703

Merged
housseindjirdeh merged 8 commits into
gitpoint:masterfrom
Arjun-sna:language_selection
Jan 28, 2018
Merged

refactor(ui): update language selectionscreen #703
housseindjirdeh merged 8 commits into
gitpoint:masterfrom
Arjun-sna:language_selection

Conversation

@Arjun-sna
Copy link
Copy Markdown
Contributor

@Arjun-sna Arjun-sna commented Jan 23, 2018

Question Response
Version? v1.4.1
Devices tested? one plus5, Asus Zenfone
Bug fix? no
New feature? no
Includes tests? no
All Tests pass? yes
Related ticket? #620

Screenshots

Before After
before after

Description

Added a new screen for updating user language settings

move language settings to a new page

620
@Arjun-sna Arjun-sna changed the title Language selection refactor(ui): update language selectionscreen Jan 23, 2018
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 23, 2018

Coverage Status

Coverage remained the same at 43.951% when pulling 970f02d on Arjun-sna:language_selection into 00f5a09 on gitpoint:master.

update title when user selects a language
Copy link
Copy Markdown
Member

@machour machour left a comment

Choose a reason for hiding this comment

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

Thank you @Arjun-sna, this is beautiful! 🎉

Some cosmetics:

  • Could you set the background color of the new screen to white ?
  • Can you make the list item a little bigger in height (same height as the item with the checkbox mark, so that there are no differences)

Let us know if you need any help!

set color and height of language list item consistent with rest of the app
Copy link
Copy Markdown
Contributor

@ocarreterom ocarreterom left a comment

Choose a reason for hiding this comment

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

Some questions... :)

{emojifyText(item.emojiCode)}
</Text>
<Text style={styles.listTitle}>{item.name}</Text>
</View>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use styled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

<ViewContainer>
<FlatList
data={languages}
renderItem={({ item }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Go to an independent function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@machour
Copy link
Copy Markdown
Member

machour commented Jan 27, 2018

@Arjun-sna I just merged #702 which migrated the user options screen to styled component.
I also took care of fixing the conflict for your PR.

Could you follow @ocarreterom sound advice and migrate language-setting.screen.js to be completely using styled components? 🙏

@machour
Copy link
Copy Markdown
Member

machour commented Jan 27, 2018

Wonderfull work, thank you @Arjun-sna !

Copy link
Copy Markdown
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

This is so great @Arjun-sna, thank you and WELCOME <3

Comment thread .all-contributorsrc
},
{
"login": "Arjun-sna",
"name": "Arjun",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎉

@housseindjirdeh housseindjirdeh merged commit ca6d011 into gitpoint:master Jan 28, 2018
@housseindjirdeh
Copy link
Copy Markdown
Member

And thanks a million for reviewing @machour @ocarreterom 🙏

@Arjun-sna Arjun-sna deleted the language_selection branch December 14, 2018 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants