Feature/longpress #55

erikhaandrikman · 2019-09-26T12:48:29Z

Resolve #48

g-zachar · 2019-10-04T08:45:44Z

src/platforms/browser/WebPlatform.mjs

+    }
+
+    registerKeyupHandler(keyhandler){
+        this._keyupListener = e => {


Not necessarily a style issue, but I'd prefer this arrow function to have () around arguments.

g-zachar · 2019-10-04T08:47:48Z

src/application/Application.mjs

+    }
+
+    /**
+     * register and starts a timer for the pressed key. Timer will be cleared when the key is released


Typo: Registers and starts (...)

g-zachar · 2019-10-04T08:48:07Z

src/application/Application.mjs

+     * before the timer goes off.
+     *
+     * If key is not release (keyup) the longpress handler will be fired.
+     * configuration can be via the Components template:


Typo: Configuration

g-zachar · 2019-10-04T08:58:21Z

src/application/Application.mjs

+        const focusPath = this.__getFocusPath();
+        const consumer = focusPath.pop();


I it possible to use this.focusPath instead to avoid recalculation?

Good point, we can indeed grab the current calculated focus path

g-zachar · 2019-10-04T09:07:20Z

src/application/Application.mjs

        this._updateAttachedFlag();
        this._updateEnabledFlag();
+
+        if (this.__keypressTimers) {


Since this.__keypressTimers is a Map object created in constructor and never reassigned, this will always yield true. You may also want to call this._keypressTimers.clear() to get rid of dangling references.

valid point, changed the condition to validate map size and clear the map

g-zachar · 2019-10-04T09:27:26Z

tests/keyhandler/test.keyhandler.js

+        class TestApp extends lng.Application {
+            static _template(){
+                return {
+                    A:{type: ComponentA}


Style: missing space after A:

g-zachar · 2019-10-04T09:27:58Z

tests/keyhandler/test.keyhandler.js

+    });
+
+    describe('Component', function() {
+        it('should handle keydown', function(){


Style: missing space after ()

g-zachar · 2019-10-04T09:28:06Z

tests/keyhandler/test.keyhandler.js

+            chai.assert(a.handled.indexOf("_handleUp") !== -1);
+        });
+
+        it('should handle keyup', function(){


g-zachar · 2019-10-04T09:28:13Z

tests/keyhandler/test.keyhandler.js

+            chai.assert(a.handled.indexOf("_handleUpRelease") !== -1);
+        });
+
+        it('should handle captureKey', function(){


g-zachar · 2019-10-04T09:29:51Z

tests/keyhandler/test.keyhandleLong.js

+            static _template(){
+                return {
+                    A:{
+                        type: ComponentA,
+                        // configuration how long a key should be pressed before it's longpress counterpart fires
+                        longpress:{up:700, down:600, left:800, right:900}
+                    }
+                }
+            }


Inconsistent styling.

erikhaandrikman · 2019-10-09T06:15:35Z

Code style updated

g-zachar · 2019-10-09T08:17:32Z

src/application/Application.mjs

+
+        this.updateFocusPath();
+
+        const consumer = [...path].pop();


Isn't path[path.length - 1] simpler?

good point, is updated now

g-zachar · 2019-10-09T08:22:19Z

tests/keyhandler/test.keyhandleLong.js

+
+    before(() => {
+        class TestApp extends lng.Application {
+            static _template(){


Style: missing space before {

g-zachar · 2019-10-09T08:23:14Z

tests/keyhandler/test.keyhandleLong.js

+    });
+
+    describe('Component', function() {
+        const repeat = (iterations)=>{


Style: missing spaces before and after =>

g-zachar · 2019-10-09T08:23:25Z

tests/keyhandler/test.keyhandleLong.js

+
+    describe('Component', function() {
+        const repeat = (iterations)=>{
+            return new Promise((resolve)=>{


g-zachar · 2019-10-09T08:24:17Z

tests/keyhandler/test.keyhandler.js

+            }
+        }
+
+        class ComponentA extends lng.Component{


Style: missing space before {

g-zachar · 2019-10-09T08:25:47Z

src/application/Application.mjs

+        if (config[lookup]) {
+            const timeout = config[lookup];
+            if (!Utils.isNumber(timeout)) {
+                element._throwError("config value for longpress must be a number")


Style: Missing ;

g-zachar · 2019-10-09T08:26:16Z

src/application/Application.mjs

+            if (!Utils.isNumber(timeout)) {
+                element._throwError("config value for longpress must be a number")
+            } else {
+                this.__keypressTimers.set(key,setTimeout(()=>{


Style: missing spaces between and after =>

g-zachar · 2019-10-09T08:27:04Z

src/platforms/browser/WebPlatform.mjs

+        window.addEventListener('keydown', this._keydownListener);
+    }
+
+    registerKeyupHandler(keyhandler){


Style: missing space before {

g-zachar · 2019-10-09T08:27:50Z

tests/keyhandler/test.keyhandleLong.js

+
+        class ComponentA extends lng.Component {
+            static _template() {
+                return {}


Style: missing ;

g-zachar

Updated comments

erikhaandrikman added 5 commits September 18, 2019 17:36

feat(Lightning): handle keyup events

b0db2cc

feat(Longpress): support key longpress

97539c1

update(Longpress): confirm to code styling

0f8f453

update(Longpress): more code styling

0bf7b52

update(Application): cleanup longpress timers on app destroy

cbf27c3

woutermeek requested a review from g-zachar October 3, 2019 17:16

g-zachar suggested changes Oct 4, 2019

View reviewed changes

erikhaandrikman added 3 commits October 8, 2019 22:49

update(Longpress): confirm to code style

40e3b18

update(Application): proper timer check

95eeefd

update(Application): avoid focus recalculation

1617085

g-zachar reviewed Oct 9, 2019

View reviewed changes

tests/keyhandler/test.keyhandler.js Outdated

}

}

class ComponentA extends lng.Component{

Copy link

Contributor

g-zachar Oct 9, 2019

Choose a reason for hiding this comment

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

Style: missing space before {

g-zachar reviewed Oct 9, 2019

View reviewed changes

g-zachar suggested changes Oct 9, 2019

View reviewed changes

erikhaandrikman and others added 6 commits October 9, 2019 08:34

update(Application): simplify consumer assignment

855b2c6

Merge branch 'master' into feature/Longpress

d28898d

Style and bugfix in tests.

ced59b1

Style fixes

7c02e38

More style fixes

9368a61

Final style fix

be965fe

g-zachar merged commit 0971296 into master Oct 10, 2019

g-zachar deleted the feature/Longpress branch November 5, 2019 08:57

		const focusPath = this.__getFocusPath();
		const consumer = focusPath.pop();

Feature/longpress #55

Feature/longpress #55

Uh oh!

Conversation

erikhaandrikman commented Sep 26, 2019 • edited by g-zachar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikhaandrikman commented Oct 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g-zachar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erikhaandrikman commented Sep 26, 2019 •

edited by g-zachar

Loading