Skip to content
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

feat: add / remove phone number #2488

Merged
merged 5 commits into from
Jul 21, 2023
Merged

Conversation

nicolasburtey
Copy link
Member

No description provided.

@nicolasburtey nicolasburtey force-pushed the feat--add-/-remove-phone-number branch 2 times, most recently from d95d13d to 4c02313 Compare July 19, 2023 14:59
@nicolasburtey nicolasburtey marked this pull request as ready for review July 19, 2023 15:47
@nicolasburtey nicolasburtey force-pushed the feat--add-/-remove-phone-number branch from 789a73b to fb281ec Compare July 20, 2023 10:43
@@ -842,7 +842,7 @@ const en: BaseTranslation = {
learn: "I don't mean to badger you, but there's lot more to learn, dig in...",
learnToEarn: "Learn to Earn",
},
PhoneInputScreen: {
PhoneLoginInitiateScreen: {
Copy link
Contributor

Choose a reason for hiding this comment

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

In may make sense to unify PhoneLoginInitiateScreen and PhoneRegistrationInitiateScreen to PhoneAuthInitiateScreen just because they seem to be almost identical. Same with PhoneLoginValidationScreen and PhoneRegistrationValidationScreen.

@@ -723,7 +723,7 @@
"learn": "Ñuqaqa kaniyta mana yachanakuyta munanki, hinaspak kaniyta kawsayniyta kaniy, ch'usaq kaniyta...",
"learnToEarn": "Yachanakuyta kaniyta kawsayniyta kaniy"
},
"PhoneInputScreen": {
"PhoneLoginInitiateScreen": {
Copy link
Contributor

Choose a reason for hiding this comment

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

These manual fixes will get overwritten by transifex the next time we sync the languages. Perhaps there is a control in transifex to just rename a field.

@@ -124,16 +125,14 @@ export const PhoneInputScreen: React.FC = () => {
setCountryCode,
supportedCountries,
loadingSupportedCountries,
} = useRequestPhoneCode({
skipRequestPhoneCode: appConfig.galoyInstance.name === "Local",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional to remove the local bypass? How will a user go through the phone flow locally?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still here, just not on the screen but in the hook

@UncleSamtoshi
Copy link
Contributor

It looks like most of the Registration is copy and pasted from the Login flow. I think this will cause a longer term maintainability problem and it would be beneficial to either adapt the Login flow to handle registration or extract the common logic/display between the two.

@nicolasburtey
Copy link
Member Author

It looks like most of the Registration is copy and pasted from the Login flow. I think this will cause a longer term maintainability problem and it would be beneficial to either adapt the Login flow to handle registration or extract the common logic/display between the two.

I have followed Justin heuristics of "if you have something only twice do not refactor but instead copy and you can later see a pattern when you have a third use"

So yeah there is some duplication but I don't know if this needs a refactoring now.

The flow diverges in:

  • the network calls
  • use of captcha or not
  • error returned

the rest is fairly identical indeed

No strong opinion here but if you strongly feel I should refactor I can do another pass

@UncleSamtoshi
Copy link
Contributor

Could be a topic for a code review, but I don't have a strong opinion at the moment either, so I will approve.

@sandipndev
Copy link
Member

image

It's strange to me that Remove Email is it's own button while remove phone number has a cross mark.

@nicolasburtey nicolasburtey merged commit eb49400 into main Jul 21, 2023
7 checks passed
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.

3 participants