Skip to content

PER-9982 hitting enter submit 2fa code#530

Merged
crisnicandrei merged 5 commits intomainfrom
PER-9982-hitting-enter-submit-2fa-code
Feb 17, 2025
Merged

PER-9982 hitting enter submit 2fa code#530
crisnicandrei merged 5 commits intomainfrom
PER-9982-hitting-enter-submit-2fa-code

Conversation

@crisnicandrei
Copy link
Contributor

Add new method to submit 2fa code when user hits

Steps to test:
Generate a 2fa code via email or sms
Add it in the input field and then press eneter
It should add/remove the method

}}"
[control]="form.controls['contactInfo']"
[disabled]="!!selectedMethodToDelete"
(keydown.enter)="onEnterPress($event, 'sendCode')"
Copy link
Contributor

Choose a reason for hiding this comment

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

In general we should avoid mapping a literal keydown.enter event to these inputs. While the ticket specifically calls out hitting the enter key, we should be using more generalized solutions. Specifically: using a form submit event instead. Mobile keyboards often have some kind of submit functionality built into them and they usually work by submitting the form itself rather than actually triggering an enter key press. So this solution might not work as smoothly on mobile devices.

Right now we have two different inputs with different purposes under one form, which is causing the existing bug. But I think semantically it makes sense for these inputs to be in two separate forms since they have completely different purposes and actions upon submitting. This lets us hit enter/submit/etc on each input and have them do separate things.

@crisnicandrei
Copy link
Contributor Author

@meisekimiu replaced the form with 2 forms. Do you think there should be tests for that?

@codecov
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.78%. Comparing base (6fc5aef) to head (eef846d).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #530   +/-   ##
=======================================
  Coverage   43.77%   43.78%           
=======================================
  Files         366      366           
  Lines       11215    11215           
  Branches     1841     1841           
=======================================
+ Hits         4909     4910    +1     
- Misses       6140     6142    +2     
+ Partials      166      163    -3     

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

Copy link
Contributor

@meisekimiu meisekimiu 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 fine! There are already existing tests that run sendCode and submitData so we don't really need to test them through the UI.

@crisnicandrei crisnicandrei requested a review from k8lyn6 February 12, 2025 20:57
Copy link
Collaborator

@yeslikesolo yeslikesolo left a comment

Choose a reason for hiding this comment

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

Everything looks great! :D

@crisnicandrei crisnicandrei force-pushed the PER-9982-hitting-enter-submit-2fa-code branch from f1f45fe to 052413f Compare February 14, 2025 08:34
@crisnicandrei crisnicandrei merged commit 6c60d93 into main Feb 17, 2025
4 checks passed
@crisnicandrei crisnicandrei deleted the PER-9982-hitting-enter-submit-2fa-code branch February 17, 2025 08:34
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