feat(FEC-9481): distributed keyboard handler to decorator #464

Yuvalke · 2019-12-05T18:34:11Z

Description of the Changes

create keyboard decorator event to implement in each component which use keyboard event on player area.

Solves: FEC-9481

CheckLists

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
test are passing in local environment
Travis tests are passing (or test results are not worse than on master branch :))
Docs have been updated

create keyboard decorator event to implement in each compnent which use keyboard event on player area.

src/components/keyboard/index.js

src/components/keyboard/keyboard-event-provider.js

yairans · 2019-12-25T15:18:00Z

src/components/keyboard/keyboard-event-provider.js

+   * @private
+   */
+  _shouldHandledKeyboardEvent(): boolean {
+    return this.props.isKeyboardEnable && !this.props.shareOverlay && !this.props.playerNav;


share overlay can set isKeyboardEnable: false by itself instead of this, no?

No, if user will change the isKeyboardEnable to false and then share will change it to false as well and then true the final state of isKeyboardEnable will be true and that's not what the user wants.
There is another solution that could contain internalKeyboard state and externalKeyboard state but I don't think it's needed here.

src/components/keyboard/with-keyboard-event.js

src/components/keyboard/keyboard-event-provider.js

src/components/keyboard/with-keyboard-event.js

yairans · 2019-12-25T19:23:55Z

src/components/language/language.js

@@ -76,6 +79,38 @@ class Language extends Component {
   */
  componentDidMount() {
    this.props.eventManager.listen(document, 'click', e => this.handleClickOutside(e));
+    this.props.addKeyboardHandler({code: KeyMap.C}, event => {
+      event.preventDefault();


import this handler from keyboard.js. we can't maintain code like this twice.
same for all new handlers in this change

we'll not.
keyboard.js is deprecated if someone wants to use it he'll need to write the logic by himself I don't think we need to maintain this code anymore.

Yuvalke · 2019-12-25T13:27:21Z

src/components/keyboard/keyboard-event-provider.js

+    if (!this._keyboardListeners[keyCode]) {
+      this._keyboardListeners[keyCode] = callback;
+    }
+    window.console.log(this._keyboardListeners[keyCode]);


Yuvalke · 2019-12-26T08:05:09Z

src/components/keyboard/keyboard-event-provider.js

+   * @private
+   */
+  _shouldHandledKeyboardEvent(): boolean {
+    return this.props.isKeyboardEnable && !this.props.shareOverlay && !this.props.playerNav;


No, if user will change the isKeyboardEnable to false and then share will change it to false as well and then true the final state of isKeyboardEnable will be true and that's not what the user wants.
There is another solution that could contain internalKeyboard state and externalKeyboard state but I don't think it's needed here.

Yuvalke · 2019-12-26T08:09:07Z

src/components/language/language.js

@@ -76,6 +79,38 @@ class Language extends Component {
   */
  componentDidMount() {
    this.props.eventManager.listen(document, 'click', e => this.handleClickOutside(e));
+    this.props.addKeyboardHandler({code: KeyMap.C}, event => {
+      event.preventDefault();


we'll not.
keyboard.js is deprecated if someone wants to use it he'll need to write the logic by himself I don't think we need to maintain this code anymore.

…nto FEC-9481

OrenMe · 2019-12-31T12:43:13Z

src/components/keyboard/keyboard-event-provider.js

+ * @extends {Component}
+ */
+class KeyboardEventProvider extends Component {
+  _events: Object = {keydown: 1, keyup: 2, keypress: 3};


keypress is deprecated, please remove
https://developer.mozilla.org/en-US/docs/Web/API/Document/keypress_event

OrenMe · 2019-12-31T12:46:20Z

src/components/keyboard/keyboard-event-provider.js

+   * @returns {void}
+   */
+  componentWillUnmount(): void {
+    const {eventManager, playerContainer} = this.props;


withEventManager destroys the event manager on componentWillUnmount, so no need for this

OrenMe · 2019-12-31T13:59:22Z

src/components/keyboard/with-keyboard-event.js

+     * @memberof withKeyboardEvent
+     */
+    render(): React$Element<any> | void {
+      return <WrappedComponent {...this.props} registerEvents={eventHandlers => this.registerEvents(eventHandlers)} />;


registerEvents as an API is too generic name.
If we are passing this as named prop it should be understood it is addKeyboardBindings or addKeyboardEvent

OrenMe · 2019-12-31T14:01:53Z

src/components/language/language.js

-
+  _keyboardEventHandlers: Array<KeyboardEventHandlers> = [
+    {
+      eventType: 'keydown',


each event should have
{
eventType?,
key,
handler
}

cause each key binding can have different eventType potentially
Also, eventType should be optional cause keyDown is the default

I grouped by eventType to make it clear
[{eventType:keydown,
handlers:[]}]

Handler -> {key:keyboardKey, action:function}

…nto FEC-9481

changes done

OrenMe · 2020-01-01T08:40:49Z

src/components/keyboard/keyboard-event-provider.js

+
+const COMPONENT_NAME = 'KEYBOARD_PROVIDER';
+
+export const keyboardEvents: Object = {keydown: 1, keyup: 2};


ENUM is CAPITAL snake case KEYBOARD_EVENTS KEYDOWN KEYUP

feat(FEC-9481): Distributed Keyboard handler to decorator

43330a5

create keyboard decorator event to implement in each compnent which use keyboard event on player area.

Yuvalke added the Type: Feature label Dec 5, 2019

Yuvalke requested review from OrenMe, yairans and RoyBregman December 5, 2019 18:34

Yuvalke self-assigned this Dec 5, 2019

Yuvalke added 10 commits December 5, 2019 20:37

delete keyboard element

385e5ff

add missing events

b1233b2

rewrite to es6 format

374e7d4

Update vr-stereo.js

0120778

fix keycode format

ba61a6e

add prop for disable enable

1c8d427

Update keyboard to be handled by his prop

e160793

Merge branch 'master' into FEC-9481

b0c3208

Revert change on event dispather for backward capability

ce87c78

Update keyboard-event-provider.js

c0702ed

yairans previously requested changes Dec 25, 2019

View reviewed changes

Yuvalke commented Dec 26, 2019

View reviewed changes

Yuvalke and others added 12 commits December 26, 2019 10:18

fix docs and expose KeyboardEventProvider

623a9f5

Add remove handler for unmount elements

726be0f

Fix for preset which doesn't include seekbar it's not relevant anymore

97612be

Update seekbar.js

8c1b782

docs and expose the new decorator

c475a60

update implemention

e9700c5

update type name

d3698d9

fix flow and eslint

801bcb4

Merge branch 'master' into FEC-9481

3978e8b

fix docs and seek for accessability

0194e57

Merge branch 'FEC-9481' of https://github.com/kaltura/playkit-js-ui i…

baa203f

…nto FEC-9481

Update language.js

50faf5a

update preset name

4417f6c

OrenMe requested changes Dec 31, 2019

View reviewed changes

change event structure and keeping the states on the provider only

fa96620

Yuvalke changed the title ~~feat(FEC-9481): Distributed Keyboard handler to decorator~~ feat(FEC-9481): distributed keyboard handler to decorator Jan 1, 2020

Yuvalke and others added 4 commits January 1, 2020 16:51

Merge branch 'master' into FEC-9481

16bac4a

remove unnessery code and use lifecycle methods

9bd805a

Merge branch 'FEC-9481' of https://github.com/kaltura/playkit-js-ui i…

ce9bf7f

…nto FEC-9481

fix names

dede612

yairans self-requested a review January 2, 2020 09:17

OrenMe previously approved these changes Jan 2, 2020

View reviewed changes

Merge branch 'master' into FEC-9481

898d36f

Yuvalke dismissed OrenMe’s stale review via 898d36f January 2, 2020 12:27

OrenMe approved these changes Jan 2, 2020

View reviewed changes

Yuvalke merged commit af71c20 into master Jan 2, 2020

Yuvalke deleted the FEC-9481 branch January 2, 2020 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(FEC-9481): distributed keyboard handler to decorator #464

feat(FEC-9481): distributed keyboard handler to decorator #464

Yuvalke commented Dec 5, 2019 •

edited by OrenMe

yairans Dec 25, 2019

Yuvalke Dec 26, 2019 •

edited

yairans Dec 25, 2019

Yuvalke Dec 26, 2019

Yuvalke Dec 25, 2019

Yuvalke Dec 26, 2019 •

edited

Yuvalke Dec 26, 2019

OrenMe Dec 31, 2019

OrenMe Dec 31, 2019

OrenMe Dec 31, 2019

OrenMe Dec 31, 2019

Yuvalke Dec 31, 2019

Yuvalke Dec 31, 2019

OrenMe Jan 1, 2020


		const COMPONENT_NAME = 'KEYBOARD_PROVIDER';

		export const keyboardEvents: Object = {keydown: 1, keyup: 2};

feat(FEC-9481): distributed keyboard handler to decorator #464

feat(FEC-9481): distributed keyboard handler to decorator #464

Conversation

Yuvalke commented Dec 5, 2019 • edited by OrenMe

Description of the Changes

CheckLists

Choose a reason for hiding this comment

Yuvalke Dec 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke Dec 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuvalke commented Dec 5, 2019 •

edited by OrenMe

Yuvalke Dec 26, 2019 •

edited

Yuvalke Dec 26, 2019 •

edited