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

Fixes issue where next button doesn't automatically focus #553

Open
wants to merge 4 commits into
base: master
from

Conversation

@danielimmke
Copy link

danielimmke commented Oct 4, 2019

This addresses issue #551 - basically the DOM element isn't ready to receive the focus yet so I do a check and add a timer delay of 1 millisecond. This fixes the issue - tested in Chrome and Firefox.

src/modules/scope.js Outdated Show resolved Hide resolved
danielimmke added 2 commits Oct 6, 2019
…slight delay to test to stop a false error from occurring.
Copy link
Owner

gilbarbara left a comment

Some questions

@@ -102,6 +102,13 @@ export default class Scope {
window.removeEventListener('keydown', this.handleKeyDown);
};

checkFocus = (target: HTMLElement) => {

This comment has been minimized.

Copy link
@gilbarbara

gilbarbara Oct 31, 2019

Owner

If the target is not found won't it create an infinite loop?
How to handle that?

This comment has been minimized.

Copy link
@danielimmke

danielimmke Oct 31, 2019

Author

The check for target existing is on L:119 - I think that would prevent an infinite loop.

@@ -167,7 +167,10 @@ describe('modules/scope', () => {
});

it('should have focused the selector', () => {
expect(wrapper.find('.primary').instance() === document.activeElement).toBeTrue();

This comment has been minimized.

Copy link
@gilbarbara

gilbarbara Oct 31, 2019

Owner

The test wasn't breaking so is this change valid at all?

This comment has been minimized.

Copy link
@danielimmke

danielimmke Oct 31, 2019

Author

The test was breaking with adding any form of delay with focusing. The test environment didn't have the same issue as browsers and was able to apply focus to the DOM element immediately. Since now there is a very slight delay in adding focus, I set a wait to 100 milliseconds to get it to fire.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.