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

fix: you have to press the login button twice (#6633) #6663

Merged

Conversation

tibi77
Copy link
Contributor

@tibi77 tibi77 commented Jun 21, 2023

Description

When a user wants to login into the wallet they need to press the login button twice. The issue comes from the way that the Scrollview component is behaving when input is active.

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue
Fix: #6633

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@tibi77 tibi77 requested a review from a team as a code owner June 21, 2023 18:31
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@digiwand digiwand added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jun 23, 2023
tommasini
tommasini previously approved these changes Jul 4, 2023
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @tibi77! LGTM!

@hesterbruikman hesterbruikman added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Jul 4, 2023
@hesterbruikman
Copy link
Contributor

@tibi77 Added a Needs QA label, please stay tuned for QA feedback. I think it'll be good to do a quick check on physical device (Android + iOS)

leotm
leotm previously approved these changes Jul 5, 2023
Copy link
Contributor

@leotm leotm left a comment

Choose a reason for hiding this comment

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

(see: #6663 (review))

edit: added Fix: prefix in description to auto-close your linked issue @tibi77 thx again

tommasini
tommasini previously approved these changes Jul 5, 2023
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM with the latest commit

@tibi77
Copy link
Contributor Author

tibi77 commented Jul 11, 2023

So I can merge it @leotm ?
My first contribution to this repo, not sure how it goes :D

@tibi77
Copy link
Contributor Author

tibi77 commented Sep 11, 2023

@leotm could you approve the workflow so we can merge this

@cortisiko
Copy link
Member

@tibi77 can you test this PR and provide recording details of what was tested?

@hesterbruikman
Copy link
Contributor

@tibi77 bumping @cortisiko's comment, would love to see this included, but we'd need a recording of testing before this can be merged

@tibi77
Copy link
Contributor Author

tibi77 commented Oct 9, 2023

@hesterbruikman just saw this
recording now

@tibi77
Copy link
Contributor Author

tibi77 commented Oct 9, 2023

Screen.Recording.2023-10-09.at.21.10.45.mov

The issue was that the button for login/unlock had to be pressed twice, now, with this change you need to press the button only once
The reason that this bug happened was that the keyboard aware scroll swallows the click event and the button does not get that event, so, to fix it, we remove the event handling from the keyboard and we closed the Keyboard manually.

You can't really see it on the video but i m pressing twice the button, the keyboard goes down and only than i can press on the button

@tibi77
Copy link
Contributor Author

tibi77 commented Nov 15, 2023

@tommasini @leotm solved the conflicts, and requested the review again

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

@hesterbruikman
Copy link
Contributor

hesterbruikman commented Nov 15, 2023

You're ready to merge @tibi77 ! Thanks so much for your contribution and patience! Looking forward to see this improvement in the app

cc @chrisleewilcox

@sethkfman sethkfman added the release-7.12.0 Issue or pull request that will be included in release 7.12.0 label Nov 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e45d196) 40.44% compared to head (f1da997) 40.43%.

Files Patch % Lines
app/components/Views/Login/index.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6663      +/-   ##
==========================================
- Coverage   40.44%   40.43%   -0.01%     
==========================================
  Files        1239     1239              
  Lines       29975    29976       +1     
  Branches     2875     2875              
==========================================
  Hits        12122    12122              
- Misses      17156    17157       +1     
  Partials      697      697              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leotm leotm removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Nov 16, 2023
@leotm
Copy link
Contributor

leotm commented Nov 29, 2023

@sethkfman can you see why GH's still expecting/waiting on the status of the final SonarCloud check?
even tho summary shows a pass and the job's marked complete after we skip it (same after re-runs)

Screenshot 2023-11-29 at 3 56 23 pm

remembering from recent sonar PRs we skipped it for ext contributors in #7086
but still got couple more ext contributor PRs hanging about on this step

tried manually merging via command line but ofc our branch protection doesn't allow

@tibi77
Copy link
Contributor Author

tibi77 commented Jan 16, 2024

@leotm I still see sonar not passing... what would be the next steps to solve this?

@hesterbruikman
Copy link
Contributor

cc @frankvonhoven

@tibi77
Copy link
Contributor Author

tibi77 commented Jan 19, 2024

it seems that we have just Bitrise failing, but it looks like it fails from the get go

@hesterbruikman hesterbruikman removed the release-7.12.0 Issue or pull request that will be included in release 7.12.0 label Jan 24, 2024
@frankvonhoven frankvonhoven changed the base branch from main to temp/validate-ext-pr-login-fix January 25, 2024 19:44
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Jan 25, 2024
@sethkfman
Copy link
Contributor

This PR looks good we are merging into a temp branch were E2E testing can be run for final validation.

@sethkfman sethkfman merged commit aea73a4 into MetaMask:temp/validate-ext-pr-login-fix Jan 25, 2024
26 of 27 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
external-contributor INVALID-PR-TEMPLATE PR's body doesn't match template needs-dev-review PR needs reviews from other engineers (in order to receive required approvals)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

You have to press the login button twice.