-
Notifications
You must be signed in to change notification settings - Fork 11
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(dial-pad): prevent focus loss after removal of delete button (VIV-2126) #1912
Conversation
YonatanKra
commented
Sep 19, 2024
- Add blur listener to the dial pad
- Type 1 number
- Delete number using the backspace button in the text-field
- See that focus remains inside the dial pad
…nage/vivid-3 into VIV-2065-dial-pad-events-leak
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1912 +/- ##
===========================================
Coverage 100.00% 100.00%
===========================================
Files 123 353 +230
Lines 1562 6976 +5414
Branches 108 902 +794
===========================================
+ Hits 1562 6976 +5414
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
document.addEventListener( | ||
'blur', | ||
this.#blurHandlerAfterDeleteButtonRemoved, | ||
true | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be needed?
If there is actually a reason for it needs to be explained in a comment
document.addEventListener( | |
'blur', | |
this.#blurHandlerAfterDeleteButtonRemoved, | |
true | |
); | |
this._textFieldEl.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We need to prevent the focus event from firing
- This is tested
- I've wrapped the logic in a function with a descriptive name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. The component will not emit focus (or blur) when focus moves within, so this should not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solves the problem of focus leaving the component after deleting the last character. Focus does not remain inside the component hence the event being fired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand that. But how does the suggested change not resolve the issue? Focus moves to text-field, therefore no blur (or focus) will be emitted on the dial-pad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what you mean.
The focus will happen before the event dispatches.
Nice one, thanks!
…utton-blur-effect
…utton-blur-effect # Conflicts: # libs/components/src/lib/dial-pad/dial-pad.spec.ts # libs/components/src/lib/dial-pad/dial-pad.ts