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

[v4] fix focus trap #220

Merged
merged 5 commits into from
Mar 20, 2019
Merged

[v4] fix focus trap #220

merged 5 commits into from
Mar 20, 2019

Conversation

katydecorah
Copy link
Contributor

@katydecorah katydecorah commented Mar 19, 2019

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • update CHANGELOG.md with changes under master heading before merging

Fixes focus trap described in #162. This PR makes sure that _onKeyDown, if the user is tabbing though do not _clear the input because it will set the focus back on the input.

@katydecorah katydecorah changed the title fix focus trap [v4] fix focus trap Mar 19, 2019
@katydecorah katydecorah mentioned this pull request Mar 19, 2019
3 tasks
@katydecorah
Copy link
Contributor Author

@scottsfarley93 I added a test to make sure that when the input is empty and in focus and then there is a keydown event from Tab that the _clear function is not called. The _clear function inside _onKeyDown sets the focus back on the input, which is why we are getting trapped on the input.

I attempted to test under the same conditions, expect that if a different key is pressed that _clear is called. But I'm having trouble getting it to work because _onKeyDown is wrapped in debounce(). I'm not well versed in Sinon yet, but I read that we can use Sinon's timers. I'm struggling to figure it out 🙃. Do you have any suggestions on testing a debounce function or do you think the current test is sufficient for now?

@katydecorah katydecorah mentioned this pull request Mar 20, 2019
17 tasks
Copy link

@scottsfarley93 scottsfarley93 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Good work on getting the tests to cover this so we know we won't regress.

@katydecorah katydecorah merged commit ff7346a into version4 Mar 20, 2019
@katydecorah katydecorah deleted the v4-focus branch March 20, 2019 19:54
katydecorah pushed a commit that referenced this pull request Mar 20, 2019
* version4: (37 commits)
  fix focus trap (#220)
  add `npm run docs` step to template (#221)
  Fix typo in documentation
  additional properties existance check
  Ensure properties exist prior to checking id
  respond to @katydecorah 's code review comments
  update changelog
  deduplicate result event emissions
  update changelog
  remove use of hardcoded feature IDs
  update outdated packages with npm update
  update changelog
  Bump insecure dev dependencies
  tests for options.flyTo
  update changelog
  bump suggestions to v1.3.4
  Update changelog
  update jsdoc and api docs
  pass flyTo options to map upon selection
  extend eslint, add precommit hook (#210)
  ...
katydecorah pushed a commit that referenced this pull request Mar 20, 2019
* version4: (25 commits)
  fix focus trap (#220)
  add `npm run docs` step to template (#221)
  Fix typo in documentation
  additional properties existance check
  Ensure properties exist prior to checking id
  update changelog
  remove use of hardcoded feature IDs
  update outdated packages with npm update
  update changelog
  Bump insecure dev dependencies
  tests for options.flyTo
  Update changelog
  update jsdoc and api docs
  pass flyTo options to map upon selection
  extend eslint, add precommit hook (#210)
  replace sha.js with nanoid reduce bundle size to 42kb (13kb gzipped)
  [docs] update deploy process (#198)
  prepare to publish v3.1.6
  update package-lock
  bump package version to v3.1.5
  ...
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