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

make phone number field editable #804

Merged
merged 9 commits into from
Jan 8, 2025

Conversation

AlaaElattar
Copy link
Contributor

@AlaaElattar AlaaElattar commented Dec 11, 2024

Changes

  • make phone number field editable.

Related Issues

Tested Scenarios

2024-12-22.15.49.57.mp4
2024-12-22.15.50.41.mp4
2024-12-22.15.51.09.mp4
  • Tried to add add phone for the first time and verify it, whatever the phone is verified or not it can be edited easily and then verified again.
  • If user tried to update with the same old number, will get an error message.
  • Multiple user verify with the same phone number (phone numbers not unique around all users)
  • When multiple users verified with the same number, if one of them edit and change his phone number this shouldn't affect the other users phone number which are previously verified.

@AlaaElattar AlaaElattar marked this pull request as draft December 11, 2024 10:56
@AlaaElattar AlaaElattar changed the title WIP: make phone number field editable make phone number field editable Dec 22, 2024
@AlaaElattar AlaaElattar marked this pull request as ready for review December 22, 2024 14:01
Copy link
Contributor

@AhmedHanafy725 AhmedHanafy725 left a comment

Choose a reason for hiding this comment

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

  • Please add edit icon so the user can know that he can edit the phone number
  • Prevent using the same old phone number

Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

  • Search term color is not compatible with dark theme.

image

  • Search result is not accurate. I tried searching with eg and got the following result.

  • Also, notice at the bottom of the page, an overflow happens.

  • Padding all should be increased in the dialogue

image

  • While editing the phone, the username and icon shows at the bottom again. Maybe we can check a better UI for this?

image

@zaelgohary
Copy link
Contributor

As for the overflow, I opened an issue for it here

child: Padding(
padding: const EdgeInsets.symmetric(horizontal: 16),
child: Text(
'Changing your phone will require you to go through the phone verification process again.',
Copy link
Contributor

@zaelgohary zaelgohary Dec 24, 2024

Choose a reason for hiding this comment

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

Suggested change
'Changing your phone will require you to go through the phone verification process again.',
'Changing your phone will require re-verification.',

Same with email msg.

Also, can we add more space between the msg & the input? and maybe increase space at the bottom of the cancel btn.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • All applied except the space under Cancel button, It's a custom dialog used everywhere. They should be all consistent.

@zaelgohary
Copy link
Contributor

zaelgohary commented Dec 24, 2024

  • Edited a phone then clicked verify. SMS warning didn't disappear after 2 mins. Notice the time in the screenshots below.

  • Also, the phone is not editable if verification is not completed.

image

image

@ramezsaeed
Copy link

The phone number is unique per app ?
which means can two or more users verify the same number on their apps ?

@AlaaElattar AlaaElattar marked this pull request as draft December 25, 2024 07:59
@AlaaElattar
Copy link
Contributor Author

  • Search term color is not compatible with dark theme.

image

  • Search result is not accurate. I tried searching with eg and got the following result.
  • Also, notice at the bottom of the page, an overflow happens.
  • Padding all should be increased in the dialogue

image

  • While editing the phone, the username and icon shows at the bottom again. Maybe we can check a better UI for this?

image

  • For the first 2 comments, that's a package already used and i didn't change anything in it I just called the same dialog that was used for adding mobile for the first time.

  • For the third comment about username and icon, they're not duplicated, that's the kyc verification.

@AlaaElattar
Copy link
Contributor Author

  • Edited a phone then clicked verify. SMS warning didn't disappear after 2 mins. Notice the time in the screenshots below.
  • I changed nothing related to the SMS message flow. There's a separate issue for phone counter to be like the email one.
  • I believe this should be fixed there.

@AlaaElattar AlaaElattar marked this pull request as ready for review December 26, 2024 12:44
@zaelgohary
Copy link
Contributor

  • For the first 2 comments, that's a package already used and i didn't change anything in it I just called the same dialog that was used for adding mobile for the first time.

I think we can find a better package for the mobile field. If it is not in this PR, can we open an issue for it?

  • For the third comment about username and icon, they're not duplicated, that's the kyc verification.

I understand this is the KYC verification, but the UI feels repetitive. Perhaps we could open an issue to explore a better design? One idea could be removing this section and adding a 'Not verified' label next to the username at the top.

Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

Adding a negative transform to the action buttons and changing the height of the dialogue will fix the spaces issue as shown below.

TextButton(
child: const Text(
'Cancel',
),
onPressed: () {
Navigator.pop(context);
}),
if (valid)
TextButton(onPressed: verifyButton, child: const Text('Add'))

          Transform.translate(
            offset: const Offset(0, -20),
            child: 
              Row(children: [

            TextButton(
              child: const Text(
                'Cancel',
              ),
              onPressed: () {
                Navigator.pop(context);
              }),
          if (valid)
            TextButton(onPressed: verifyButton, child: const Text('Add'))
              ],)
          
          ),

image

image

app/lib/widgets/phone_widget.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

I tried verifying with the old phone and no error msg appeared.

@AlaaElattar AlaaElattar marked this pull request as draft December 29, 2024 17:10
@ramezsaeed
Copy link

Lets add these test scenarios:

  • Verifying with the old phone and error msg should appeared.
  • Multiple user verify with the same phone number. (should succeed).
  • When multiple users verified with the same number, if one of them edit and change his phone number this shouldn't affect the other users phone number which are previously verified.

@AlaaElattar
Copy link
Contributor Author

  • For the first 2 comments, that's a package already used and i didn't change anything in it I just called the same dialog that was used for adding mobile for the first time.

I think we can find a better package for the mobile field. If it is not in this PR, can we open an issue for it?

  • For the third comment about username and icon, they're not duplicated, that's the kyc verification.

I understand this is the KYC verification, but the UI feels repetitive. Perhaps we could open an issue to explore a better design? One idea could be removing this section and adding a 'Not verified' label next to the username at the top.

  • Regarding your second comment about the KYC section, it will be removed in another pr.

@AlaaElattar
Copy link
Contributor Author

AlaaElattar commented Jan 5, 2025

Transform.translate(
offset: const Offset(0, -20),
child:
Row(children: [

  • Should be fixed
    image
    image

@AlaaElattar AlaaElattar marked this pull request as ready for review January 5, 2025 13:53
Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

I still don't see an error message when the old phone number is the same as the new one.

document_6039827747461670918.mp4

@zaelgohary
Copy link
Contributor

I still don't see an error message when the old phone number is the same as the new one.

document_6039827747461670918.mp4

Please disregard this comment; I forgot to pull the latest changes. It's working fine now.

@AlaaElattar AlaaElattar merged commit bb0a2fc into development Jan 8, 2025
1 check failed
@AlaaElattar AlaaElattar deleted the development_make_phone_editable branch January 8, 2025 15:02
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.

4 participants