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

feat(PointerEvents): using PointerEvents instead of Mouse/TouchEvents #52

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pimdewit
Copy link
Collaborator

@pimdewit pimdewit commented Oct 8, 2019

#6

  • Replaced Mouse/TouchEvents events with PointerEvents
  • removed normalizeEvent utility method (now redundant)
  • updated docstrings, removed NormalisedPointerEvent type definition

TODO:

  • Find a way to use pointer events in the current testing setup; tests fail because simulant does not recognise PointerEvents

@coveralls
Copy link

Pull Request Test Coverage Report for Build 67

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 114 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-14.5%) to 76.402%

Files with Coverage Reduction New Missed Lines %
dist/macro-carousel-test.js 114 76.4%
Totals Coverage Status
Change from base Build 62: -14.5%
Covered Lines: 440
Relevant Lines: 562

💛 - Coveralls

@ciampo
Copy link
Owner

ciampo commented Oct 9, 2019

I remember writing tests for that part (and using simulant), it was quite tricky.

Also, if we go for native pointer events, we will need to add the polyfill (given the browser support) at least to the demos. And write documentation about it

Copy link
Owner

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

The refactor looks good, but I can't approve this PR without:

  • working tests
  • PointerEvents polyfill in the demo (and a note added to the docs)

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

Successfully merging this pull request may close these issues.

None yet

3 participants