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

Allow initial focus to be provided to confirm prompt #1224

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Sep 8, 2023

This PR extends the options for confirm, including one to allow defining which of the two buttons should be initially focused.

By default it keeps focusing the "cancel" button, as that's probably the non-destructive option in most of the cases.

@acelaya acelaya requested a review from robertknight September 8, 2023 07:59
@acelaya
Copy link
Contributor Author

acelaya commented Sep 8, 2023

I'm checking the failing test. I have reproduced it locally by running it a couple of times, so I'm looking for a different way to test this.

EDIT: Seems like waitFor checking that the active element is not the body does not always resolve in 10ms. I can increase the timeout, but I'm not completely sure if that's a microtask or a macrotask, and therefore, if it will make a difference or just continue failing with the same cadence, just taking longer to do it.

EDIT 2: I found what I think is a more consistent way to wait for the button to be focused by adding a focus event listener to it.

const waitForButtonToFocus = () =>
  new Promise(resolve => getButton().addEventListener('focus', resolve));
await waitForButtonToFocus();

assert.equal(document.activeElement, getButton());

@acelaya acelaya force-pushed the confirm-initial-focus branch from 75b7650 to 218f11a Compare September 8, 2023 08:10
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #1224 (f6daf66) into main (570068e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1224   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           59        59           
  Lines          829       832    +3     
  Branches       316       318    +2     
=========================================
+ Hits           829       832    +3     
Files Changed Coverage Δ
src/util/prompts.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

📢 Have feedback on the report? Share it here.

@acelaya acelaya force-pushed the confirm-initial-focus branch 2 times, most recently from c2c8a2d to 4c18d50 Compare September 8, 2023 08:32
@acelaya acelaya force-pushed the confirm-initial-focus branch from 4c18d50 to f6daf66 Compare September 8, 2023 08:34
@robertknight
Copy link
Member

EDIT 2: I found what I think is a more consistent way to wait for the button to be focused by adding a focus event listener to it.

If there is a specific event that you can wait for rather than a timeout, that is almost always preferable.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM

@acelaya acelaya merged commit 2ad3b07 into main Sep 8, 2023
4 checks passed
@acelaya acelaya deleted the confirm-initial-focus branch September 8, 2023 09:23
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.

2 participants