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(dial-pad): prevent focus loss after removal of delete button (VIV-2126) #1912

Merged
merged 29 commits into from
Oct 22, 2024

Conversation

YonatanKra
Copy link
Contributor

  1. Add blur listener to the dial pad
  2. Type 1 number
  3. Delete number using the backspace button in the text-field
  4. See that focus remains inside the dial pad

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d61b119) to head (662a66f).
Report is 1123 commits behind head on main.

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     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Comment on lines 169 to 173
document.addEventListener(
'blur',
this.#blurHandlerAfterDeleteButtonRemoved,
true
);
Copy link
Contributor

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

Suggested change
document.addEventListener(
'blur',
this.#blurHandlerAfterDeleteButtonRemoved,
true
);
this._textFieldEl.focus();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. We need to prevent the focus event from firing
  2. This is tested
  3. I've wrapped the logic in a function with a descriptive name.

Copy link
Contributor

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.

Copy link
Contributor Author

@YonatanKra YonatanKra Oct 21, 2024

Choose a reason for hiding this comment

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

See the related issue.
When pressing the delete button (in the UI) to delete the last character in the input, it fires a blur event and the dial pad loses focus. This makes sure it leaves the focus in the dial pad.
See the descriptions:
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.

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.

Copy link
Contributor

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

Copy link
Contributor Author

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!

Yonatan Kra added 3 commits September 24, 2024 20:34
…utton-blur-effect

# Conflicts:
#	libs/components/src/lib/dial-pad/dial-pad.spec.ts
#	libs/components/src/lib/dial-pad/dial-pad.ts
@YonatanKra YonatanKra merged commit fbc115e into main Oct 22, 2024
15 checks passed
@YonatanKra YonatanKra deleted the VIV-2126-fix-delete-button-blur-effect branch October 22, 2024 09:43
@github-actions github-actions bot mentioned this pull request Oct 22, 2024
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