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

Ensure typeIn has correct key option #465

Merged
merged 2 commits into from Dec 14, 2018
Merged

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Nov 7, 2018

Currently, the typeIn() helper will only set the charCode option of the KeyboardEvent. This PR also adds the correct key option.

Copy link

@eliasmelgaco eliasmelgaco left a comment

Choose a reason for hiding this comment

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

nice work

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Hmm, I think we should use triggerKeyEvent's logic here, is there a way to refactor things into that?

@rwjblue
Copy link
Member

rwjblue commented Dec 10, 2018

@mydea - Have you had a chance to poke at my idea of reusing triggerKeyEvents logic?

@mydea
Copy link
Contributor Author

mydea commented Dec 11, 2018

I've pushed a commit making typeIn use triggerKeyEvent!

@rwjblue
Copy link
Member

rwjblue commented Dec 13, 2018

Looks like a small linting error is failing CI:

/home/travis/build/emberjs/ember-test-helpers/tests/unit/dom/type-in-test.js
  81:67  error  Delete `,`  prettier/prettier
✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

The main changes here look good to me, thank you for working on it @mydea!

@mydea
Copy link
Contributor Author

mydea commented Dec 14, 2018

Oops, fixed it and also rebased on master!

@rwjblue rwjblue merged commit 04180b2 into emberjs:master Dec 14, 2018
@rwjblue rwjblue added the bug label Jan 26, 2019
@rwjblue
Copy link
Member

rwjblue commented Sep 12, 2019

FYI - We just realized (after a bit of digging with @trabus) that we accidentally made each key event triggering wait on settled with this update (totally my fault here though). Definitely not an intentional change, I think that @trabus is working up a PR to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants