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(Keyboard): add touch events #35

Merged
merged 1 commit into from
Nov 16, 2018

Conversation

MadDeveloper
Copy link

@MadDeveloper MadDeveloper commented Nov 15, 2018

Can be enabled passing useTouchEvents option to the Keyboard constructor.

Sorry for the delay, I was working on understanding the whole library, and also I'd to work on a custom theme 😉
I don't know if there are side effects with these changes, I checked but I think you should check 😅

I'm using the native touch events api, and some browsers are incompatible, maybe it could be great to use a third library to be more compliant with all browsers (Hammer.js for exemple).

EDIT: API documentation will have to add the following option: useTouchEvents, as following:

// JavaScript
const keyboard = new Keyboard({ useTouchEvents: true })

// React
<Keyboard useTouchEvents />

Can be enabled passing `useTouchEvents` option to the Keyboard constructor.
@codecov
Copy link

codecov bot commented Nov 15, 2018

Codecov Report

Merging #35 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #35   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         364    372    +8     
  Branches      110    113    +3     
=====================================
+ Hits          364    372    +8
Impacted Files Coverage Δ
src/lib/components/Keyboard.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d87993...f8fb18e. Read the comment docs.

@hodgef
Copy link
Owner

hodgef commented Nov 15, 2018

Looks neat, thanks! I actually prefer the native/standards approach, so your addition is very in line with that. Later today I will test it more thoroughly and do the merge as well as the docs update.

As I check this out, just wanted to say that all your contributions are very welcome.

Regards

@hodgef hodgef self-assigned this Nov 15, 2018
@hodgef hodgef added the ⚡️ Enhancement New feature or request label Nov 15, 2018
@hodgef hodgef added this to the 2.9.0 milestone Nov 15, 2018
@hodgef hodgef self-requested a review November 15, 2018 16:38
@MadDeveloper
Copy link
Author

MadDeveloper commented Nov 15, 2018

I stayed in the same code logic you already started, but as webpack and babel are used to build the project, I'll do another PR later in order to reorganise the code and bring all new ecmascripts features (ES6-7) in order to reduce the code size (less code = less surface for bugs), and also a more functional vision: avoid side effects, mutations, using, when it's possible, pure functions, etc.

@hodgef
Copy link
Owner

hodgef commented Nov 15, 2018

Excellent! I'm all for it, and appreciate these improvements.

One thing I tend to obsess over is the code coverage when we run npm run test -- --coverage. So after every cleanup/change I run that and update the tests (output at ./coverage/lcov-report/index.html).

Another thing is the docs coverage (npm run docs), which runs esdoc and gives a percentage based on all the function defs and variables documented in the code through comments (output at ./docs/source.html).

All of these I would like to keep in check (This is why I don't do big cleanups often haha). So if you make a change and look out for these coverages it'd be really amazing!

@MadDeveloper
Copy link
Author

Ok, I'll have a look 😄
If your goal is to achieve a 100% code coverage, maybe you're sometimes wrong, because 100% code coverage doesn't mean 100% case coverage, and that's a huge difference, because what will make your code solid, its 100% of case coverage 😉Please have a look to this article: https://medium.com/javascript-scene/mocking-is-a-code-smell-944a70c90a6a

Really interesting, and Eric Elliott is just a master of creating/composing software knowledge! Be careful, because of him I changed my way of coding! (after I read this article, I starting to read it's book on "Composing Software", and that's magic... : https://medium.com/javascript-scene/composing-software-an-introduction-27b72500d6ea (a lot of articles))

@hodgef
Copy link
Owner

hodgef commented Nov 15, 2018

Hehe thanks for these recommendations! I care for a more petty reason actually: Certain badges and tests influence search placement, like at npms.io and npm, and maybe even GitHub (?). Also, what good is having these little badges it they are not green? 😆

I was not aware of those articles however! I will check them out :)

@hodgef
Copy link
Owner

hodgef commented Nov 16, 2018

Just tested the PR on a couple tablets and on desktop, all is good - Thanks again for this quality addition 👍

@hodgef hodgef merged commit 3086a45 into hodgef:master Nov 16, 2018
@hodgef hodgef mentioned this pull request Nov 16, 2018
@MadDeveloper MadDeveloper deleted the tactile-touch-events branch November 16, 2018 07:57
@MadDeveloper MadDeveloper restored the tactile-touch-events branch November 16, 2018 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡️ Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants