Add HotKeysHOC & HOCWrappedNode for example

[vivian-eb]

Resolve #63

This diff adds the HotKeysHOC to /lib and updates the example index.html with the newly created <HOCWrappedNode /> to show a sample implementation of the HOC. Unit tests are included.

[greena13]

Hey vivian-eb, thank you for your thorough pull request.

I have been rather over-committed of late, but I will try and get around to looking this over in the next couple of days.

[vivian-eb]

Sounds good, thank you! 👍

[greena13]

I'm just getting back into this code base after having stepped away for a while, so some of the following may be misguided or incorrect - we'll just have to work through it together as I re-familiarise myself with the code base.

I took a look at your PR and noted a few things that stood out to me while I tried to grok your changes, but I am having trouble understanding how it actually works.

I am confused about the role that keyboardActionHelper, _setAccessibilityState and _getBoundHandlers play. Could you please walk me through it?

Thanks again for taking the time to put this together.

[greena13]

I am not a particular fan of HotKeysHOC as a name for the higher order component. I've always used the convention of camelCase names for HOC to make it easier to conceptualise them as functions. This seems to be consistent with the React docs

I also don't think it's necessary to have "HOC" in the name.

I suggest something like "withHotKeys" to remain consistent with the package and component name, and apparent community HOC naming conventions - unless you have a strong case against?

[vivian-eb]

the internal name we're using for Eventbrite is withKeyboardSupport, but withHotkeys might be more consistent with the library name. I can change it to withHotkeys

[greena13]

Can we keep the library name capitalisation the same as well (withHotKeys)?

[greena13]

I want to add JSDoc comments to the React Hotkeys source code some time in the near future - could you please rewrite this comment to be consistent with the JSDoc standard?

I'm thinking of the @example tag in particular.

[vivian-eb]

added! please let me know if i can expand on anything

[greena13]

Perhaps I am missing something but I am not sure why you have chosen documentFragment as the component to use, here.

[vivian-eb]

This is to avoid applying unexpected stylistic changes to the wrapped component that could come from divs.

[greena13]

const seems more appropriate than let in this context.

[vivian-eb]

changed

[vivian-eb]

@greena13 no problem! I wrote this HOC a few months back so I really appreciate your review. Compared to how <HotKeys> currently work, the HOC has a lot of parts to it as well. The difference is that with the HOC implementation approach, 1) componentWillReceiveProps can consolidate impending state changes 2) with keyboardAction as a prop, the propType can be enforced to be PropTypes.oneOf(Object.keys(ACTION_KEY_MAP));.

I've added comments to _setAccessibilityState and _getBoundHandlers within the HOC file. _getBoundHandlers loops through the keyMap and binds each keyboardAction to _setAccessibilityState so that the action and the event gets set on the HOC state (to be passed through to the wrapped component.)

[greena13]

Hey, thanks for the updated code. I am not sure I'll get a chance to look at it today - I'm just headed out - but if you find the time, could you please merge master into your fork and address the merge conflicts?

I think it should be as simple as deleting the build directory and addressing any linting issues that pop up (npm run lint). I've changed how the package is built which no longer uses or tracks the build directory, and moved to a (hopefully) more well-adopted set of linting rules since you created this fork.

[greena13]

My plans changed somewhat so I have a few minutes to look things over.

So if I understand the code correctly, withHotKeys is a HOC (function) that returns a component that assigns handlers for each action in the keymap, bound to that component. Every time a matching keyboard event is triggered, the component updates it state (storing the event) - which causes the component to re-render and pass the new event down as props to its child.

The child (wrapped) component must then check when it's props are changed via componentWillReceiveProps to see if it has registered an action to deal with that keyboard event.

It strikes me that there are a number of inefficiencies with this approach. Every time a keyboard event that matches the HoC's keymap is triggered, the wrapped component is re-rendered, regardless of whether it has a matching handler. If that component does have a matching handler, that handler will likely involve updating its own state or the props of its children - causing a second re-render.

I am also not crazy about the wrapped component having to then check to see if a new keyboard event has occurred and whether it has a matching handler. This feels like a lot of boilerplate code and and a duplication of the functionality that HotKeys already provides internally.

Perhaps we should start back at the beginning so I can get an better idea of what the benefit of a HoC is over using the HotKeys component. Is it just to provide an alternative (less declarative syntax) or is it to remove duplication?

What are the advantages of using a HoC over two HotKeys components - one towards the top of the React render tree that defines the actions and one (or more) HotKeys that define the handlers. (See the second code box in Defining Handlers.

I believe this separation (not requiring the same HotKeys component to define both the keyMap and the actions) or decoupling of the definition of shared keyboard events and scope-dependent handlers, is one of the main strengths of react-hotkeys. This approach matches my own use of the package but I am open to other ways of using it if there seem genuine benefits to it - or situations where it is necessary.

I am also not opposed to supporting an alternative syntax (in the form of a HoC) if people want or prefer it - so long as it does not duplicate the existing functionality too much and essentially double the code that must be maintained and tested.

[vivian-eb]

@greena13 The original HOC was done a while back, and looking at it again, I realize that it did duplicate many functionalities. I've reworked the HOC and removed a lot of steps for the implementation. Please let me know what you think :)

[greena13]

Have you tested this in the browser? I have previously had issues when I've experimented with trying to change the parameters passed to HotKeys using state. I wish I could remember what happened and whether it was handlers or keyMap I was trying to change.

In any case, I came away the (potentially erroneous) understanding that HotKeys didn't support changing its arguments after it had mounted.

[vivian-eb]

Yep! This was tested using the HOCWrappedNode in the example html

[greena13]

OK, that's good enough for me. ;)

[greena13]

I don't think these additions are required. As far as I understand, everything is built into either cjs, umd or es.

[greena13]

I am not sure this is really necessary. Is there any benefit to checking the component's prop types twice - once in withHotKeysWrapper and once in Component?

[vivian-eb]

removed

[greena13]

I think the component class should be capitalised and a noun, as is consistent with JS conventions: HotKeysWrapper

[vivian-eb]

changed

[greena13]

handlers here doesn't seem to actually be used?

[vivian-eb]

removed

[greena13]

This is much better than the first version. Thanks for taking the time to rework it.

I still have mixed feelings about this - reaching into the children of withHotKeysWrapper to pick off the hotKeyHandlers attribute feels a little like information flow in the wrong direction. I guess I am still having trouble understanding a use case for this.

From what I can see, this HoC can be used to create a component that you can pass arbitrary (but necessarily singular) children to, who define their handlers later on (but they can't change after mounting without causing withHotKeysWrapper to update, somehow). So the actions stay the same - they are defined and encapsulated in the withHotKeysWrapper component, but you can re-use that component and mount it with different children, who define different handlers.

So if you were only to use that withHotKeysWrapper component once, I would say that it would be more declarative (and avoid a double render) to just use one HotKeys component at the top of the application that defines the keyMap and many different HotKeys descendants who define (different) handlers.

However, if you want to use it in many places in your application to enforce a sort of interface (children of the withHotKeysWrapper component can/should define handlers to the sequences in the withHotKeysWrapper 'skeyMap), then I can perhaps begin to understand there being a use case for this. But I am not sure if such a use case can be better handled slightly differently.

In my head I am thinking of it as this:

class MyComponent extends Component {
  render() {
    return (
      <HotKeys handlers={ myHandlers } >
        //...
      </HotKeys>
    )
  }
}

class MyContainer extends Component {
  render() {
    return {
      <HotKeys keyMap={myKeyMap} >
        //-----
        // ...|- <MyComponent />
        // ......|- <MyComponent />
        // .....|- <MyComponent />
      </HotKeys>
    }
  }
}

Being replaced by this:

class MyComponent extends Component {
  constructor(props, context) {
    super(props, context)

    this.hotKeyHandlers = myHandlers
  }

  render() {
    return (
      //...
    )
  }
}

const MyContainer = withHotKeys(myKeyMap)(MyComponent)

which does remove the reference to HotKeys in MyComponent, but it comes at the cost of - I think- double rendering as the withHotKeysWrapper component must wait to have its child to get the handlers, to re-render that child with the information that it already has.

What do you think?

[vivian-eb]

I understand the concerns of 1) getting the child's state via ref and 2) one extra render on componentDidMount. I think that these are trade-offs for 1) abstracting out <HotKeys> from the Component render and 2) sectioning handlers into hotKeyHandlers instead of defining in render (which could already be fill with other logic). Modularizing hotkeys support is advantageous for managing multiple components with different needs for keyboard support. With that said, if there's a way for the HOC to not re-render but also not duplicate HotKeys functionalities, please let me know! You have more expertise :)

[greena13]

I can't think of a way to provide a HOC that matches the functionality of the component (allowing handlers to be defined at mount-time), so this is the best solution available to us at the moment.

[greena13]

Are you able to address this warning in the test suite:

Warning: <documentFragment /> is using uppercase HTML. Always use lowercase HTML tags in React.

And investigate the failing test?

Are you also able to add TypeScript definitions for the new function to index.d.ts?

[vivian-eb]

1. I addressed the warning by making component="document-fragment"
2. Fixed failing test
3. I'm not familiar with TypeScript & index.d.ts, how should I go about this?

[greena13]

Thanks for your multiple revisions - I appreciate the time you've put into this.

I can merge and add the type definition if you want, or if you want to learn or do them yourself, we just need to declare a function (withHotKeys) that accepts a KeyMap (type already defined in latest version of index.d.ts) as an argument and returns a HotKeys instance (again, already defined in index.d.ts).

You also need to make sure that that function is declared as an export of the package.

Depending on how you learn and how much time you want to spend on this, a cheatsheet like this one will be helpful, or you can start from the top. In either case take a look at what's already there and expand upon it.

What you end up with should only be an additional line at the bottom of the file.

[vivian-eb]

I gave it a stab -- hopefully it's correct!

[greena13]

Thanks @vivian-eb, that was indeed correct (I just refactored to re-use a definition already there).

I've released that as v1.1.0