refactor: apply code review suggestions

vaadin · Sep 2, 2021 · d5ea194 · d5ea194
1 parent c5269ef
commit d5ea194
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 20 deletions.
diff --git a/packages/field-base/src/keyboard-mixin.d.ts b/packages/field-base/src/keyboard-mixin.d.ts
@@ -3,6 +3,7 @@
  * Copyright (c) 2021 Vaadin Ltd.
  * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
  */
+
 /**
  * A mixin that manages keyboard handling.
  * It subscribes to the keyboard events while providing an actual implementation
@@ -19,13 +20,13 @@ interface KeyboardMixin {
    * A handler for the `keydown` event. By default, it does nothing.
    * Override the method to implement your own behavior.
    */
-  _onKeyDown(_event: KeyboardEvent): void;
+  _onKeyDown(event: KeyboardEvent): void;
 
   /**
    * A handler for the `keyup` event. By default, it does nothing.
    * Override the method to implement your own behavior.
    */
-  _onKeyUp(_event: KeyboardEvent): void;
+  _onKeyUp(event: KeyboardEvent): void;
 }
 
 export { KeyboardMixinConstructor, KeyboardMixin };
diff --git a/packages/field-base/src/keyboard-mixin.js b/packages/field-base/src/keyboard-mixin.js
@@ -7,6 +7,7 @@ import { dedupingMixin } from '@polymer/polymer/lib/utils/mixin.js';
 
 const KeyboardMixinImplementation = (superclass) =>
   class KeyboardMixinClass extends superclass {
+    /** @protected */
     ready() {
       super.ready();
 
@@ -23,6 +24,7 @@ const KeyboardMixinImplementation = (superclass) =>
      * A handler for the `keydown` event. By default, it does nothing.
      * Override the method to implement your own behavior.
      *
+     * @param {KeyboardEvent} _event
      * @protected
      */
     _onKeyDown(_event) {
@@ -33,6 +35,7 @@ const KeyboardMixinImplementation = (superclass) =>
      * A handler for the `keyup` event. By default, it does nothing.
      * Override the method to implement your own behavior.
      *
+     * @param {KeyboardEvent} _event
      * @protected
      */
     _onKeyUp(_event) {

diff --git a/packages/field-base/test/keyboard-mixin.test.js b/packages/field-base/test/keyboard-mixin.test.js
@@ -5,39 +5,55 @@ import { fixtureSync } from '@vaadin/testing-helpers';
 import { PolymerElement, html } from '@polymer/polymer/polymer-element.js';
 import { KeyboardMixin } from '../src/keyboard-mixin.js';
 
-customElements.define(
-  'keyboard-mixin-element',
-  class extends KeyboardMixin(PolymerElement) {
-    static get template() {
-      return html`<div></div>`;
-    }
-  }
-);
-
 describe('keyboard-mixin', () => {
-  let element;
+  let element, keyDownSpy, keyUpSpy;
+
+  before(() => {
+    keyDownSpy = sinon.spy();
+    keyUpSpy = sinon.spy();
+
+    customElements.define(
+      'keyboard-mixin-element',
+      class extends KeyboardMixin(PolymerElement) {
+        static get template() {
+          return html`<div></div>`;
+        }
+
+        _onKeyDown(event) {
+          keyDownSpy(event);
+        }
+
+        _onKeyUp(event) {
+          keyUpSpy(event);
+        }
+      }
+    );
+  });
 
   beforeEach(() => {
     // Sets tabindex to 0 in order to make the element focusable for the time of testing.
     element = fixtureSync(`<keyboard-mixin-element tabindex="0"></keyboard-mixin-element>`);
-    element._onKeyDown = sinon.spy();
-    element._onKeyUp = sinon.spy();
     element.focus();
   });
 
+  afterEach(() => {
+    keyDownSpy.resetHistory();
+    keyUpSpy.resetHistory();
+  });
+
   it('should handle keydown event', async () => {
     await sendKeys({ down: 'A' });
 
-    expect(element._onKeyDown.calledOnce).to.be.true;
-    expect(element._onKeyDown.args[0][0]).to.be.an.instanceOf(KeyboardEvent);
-    expect(element._onKeyDown.args[0][0].type).to.equal('keydown');
+    expect(keyDownSpy.calledOnce).to.be.true;
+    expect(keyDownSpy.args[0][0]).to.be.an.instanceOf(KeyboardEvent);
+    expect(keyDownSpy.args[0][0].type).to.equal('keydown');
   });
 
   it('should handle keyup event', async () => {
     await sendKeys({ up: 'A' });
 
-    expect(element._onKeyUp.calledOnce).to.be.true;
-    expect(element._onKeyUp.args[0][0]).to.be.an.instanceOf(KeyboardEvent);
-    expect(element._onKeyUp.args[0][0].type).to.equal('keyup');
+    expect(keyUpSpy.calledOnce).to.be.true;
+    expect(keyUpSpy.args[0][0]).to.be.an.instanceOf(KeyboardEvent);
+    expect(keyUpSpy.args[0][0].type).to.equal('keyup');
   });
 });