LBRYCommunity/lbry-react-native

Invites page #114

Merged

akinwale merged 2 commits from invites into master

2020-01-29 12:25:24 +01:00

akinwale commented

2020-01-24 18:26:16 +01:00

(Migrated from github.com)

No description provided.

neb-b (Migrated from github.com) reviewed 2020-01-24 18:26:16 +01:00

kauffj (Migrated from github.com) requested changes 2020-01-25 00:01:46 +01:00

kauffj (Migrated from github.com) left a comment

Excludes UI changes discussed and shared here:

https://workflowy.com/s/invite-review-notes/2ql67qyEWQHPDmBr

Excludes UI changes discussed and shared here: https://workflowy.com/s/invite-review-notes/2ql67qyEWQHPDmBr

src/component/channelSelector/view.js

kauffj (Migrated from github.com) commented

2020-01-24 23:52:37 +01:00

(I didn't test create a channel flow from inside of Invites in my UI review. Please test this case explicitly before shipping.)

(I didn't test create a channel flow from inside of Invites in my UI review. Please test this case explicitly before shipping.)

src/component/drawerContent/view.js

					
				@ -22,6 +22,7 @@ const groupedMenuItems = {

				  Wallet: [

				    { icon: 'wallet', label: 'Wallet', route: Constants.DRAWER_ROUTE_WALLET },

				    { icon: 'award', label: 'Rewards', route: Constants.DRAWER_ROUTE_REWARDS },

				    { icon: 'user-friends', label: 'Invites', route: Constants.DRAWER_ROUTE_INVITES },

kauffj (Migrated from github.com) commented

2020-01-24 23:52:59 +01:00

i18n missing on all of these labels

i18n missing on all of these labels

kauffj (Migrated from github.com) commented

2020-01-24 23:55:48 +01:00

Ok, I see you're doing it in the loop below. IMO it is better to always do it directly on the string to prevent double translation and to facilitate inspection.

Ok, I see you're doing it in the loop below. IMO it is better to always do it directly on the string to prevent double translation and to facilitate inspection.

src/page/invites/view.js

kauffj (Migrated from github.com) commented

2020-01-24 23:57:47 +01:00

Where is this coming from? If IAPI, do not i18n. If internal to app, i18n when constructing error.

Where is this coming from? If IAPI, do not i18n. If internal to app, i18n when constructing error.

kauffj (Migrated from github.com) commented

2020-01-24 23:59:20 +01:00

Can we put emojis in these? Is that bad?

if not, do: "${email} was invited to the LBRY party 🥳"

Can we put emojis in these? Is that bad? if not, do: "${email} was invited to the LBRY party 🥳"

kauffj (Migrated from github.com) commented

2020-01-25 00:00:22 +01:00

i18n

i18n

kauffj (Migrated from github.com) commented

2020-01-25 00:00:57 +01:00

imaginary@friend.com?

imaginary@friend.com?

kauffj (Migrated from github.com) commented

2020-01-25 00:01:08 +01:00

i18n

i18n

src/page/invites/view.js

					
				@ -0,0 +79,4 @@

				  handleInvitePress = () => {

				    const { inviteNew, notify } = this.props;

				    const { email } = this.state;

				    if (!email || email.indexOf('@') === -1) {

kauffj (Migrated from github.com) commented

2020-01-24 23:57:12 +01:00

This is fine as a check, but this can't be the only place we validate email. There should only be one function that ascertains whether an email is valid and it should be used everywhere.

This is fine as a check, but this can't be the only place we validate email. There should only be one function that ascertains whether an email is valid and it should be used everywhere.

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-react-native#114

No description provided.