Set the inventory key binding to letter "I" and set Inventory button to be unfocusable

[KajiTetsushi]

Hi, @ASteinheiser. Forwarding my usability concerns from Reddit:
https://www.reddit.com/r/reactjs/comments/aylp2p/i_wrote_a_post_on_making_an_rpg_with_react_redux/ei4lcjt

On desktop, when I want to read my Inventory, I use the mouse & click on the button, then I have to close it with the same controls, but the Inventory button does not lose focus. Here, [Space] and [Enter] are used as attack action keys, but by browser design, they also mean a "click" user action on <button> elements, so, when I close the Inventory, then attack a monster on my next move, the Inventory comes up. It's really annoying.

There really needs to be something to tackle this:

• Set the Inventory button's tabIndex to -1.
  https://github.com/ASteinheiser/react-rpg.com/blob/master/src/features/inventory/index.js#L54

class Inventory extends Component {
  // ... other component class methods

  render() {
    const { newItemIndicator } = this.state;
    const { disabled, dialog, sideMenu } = this.props;
    const { inventory: open } = dialog;

    return (
      <div className='flex-row inventory__container'>
        {
          !disabled &&
            <Button
              ...otherButtonProps
              tabIndex={-1} // unfocusable button
            />
        }
      </div>
    );
  }
}

• Set the Inventory key binding to [I],
  https://github.com/ASteinheiser/react-rpg.com/blob/master/src/features/player/controls.js#L120

// ... other module imports

import isGamePaused    from '../dialog-manager/actions/is-game-paused';
import toggleInventory from '../dialog-manager/actions/toggle-inventory';
// inventory visibility is controlled by the global app state; good for us!

import { ANIMATION_SPEED } from '../../config/constants';

const Controls = ({ isGamePaused, attackMonster, movePlayer }) => {
  // ... other component constants & functions

  function handleKeyDown(event) {
    event.preventDefault();
    // move with 'WASD' or Arrow keys
    switch(event.keyCode) {
      case 37:
      case 65:
        return movePlayer('WEST');

      // ...
      // ... other `event.keyCode` case handlers
      // ...

      case 73:
        return toggleInventory(); // "I" => show / hide the inventory!

      default:
        // some console.log comment I couldn't be bothered to copy! :)
    }
  }

  return null;
}

const actions = {
  attackMonster,
  movePlayer,
  toggleInventory, // controls should be able to modify Inventory visibility, too!
  isGamePaused,
};

export default connect(null, actions)(Controls);

I'm sure that this will open up more questions from the audience about key binding usability, but I think it's a reasonable place to start.

[KajiTetsushi]

I didn't create a pull request to staging yet because of Contribution step 2: Discussion First and since I've never made community pull requests before, I'm very careful about what I'm doing.

In the meantime, @ASteinheiser, you can check out the changes I did on my fork:
staging...KajiTetsushi:master

[ASteinheiser]

You can go ahead and open a pull request. My intention with that second step is mainly so the community has a chance to discuss on Github before we start merging PRs.

I think this is a good change, and I imagine if we have a Stats dialog in the future, there could be a keybinding for that as well.

[KajiTetsushi]

@ASteinheiser, PR's up.

I feel there's an even better way to do what I just did 3 days ago. Check out my PR #55. Basically, inventory toggling keydown is handled inside the inventory button rather than through the player controls component, which is already quite busy.

The idea is that <Inventory /> will listen for its own key bindings rather than have some other component do the listening, so, if you decide to e.g. deprecate the button, you can do so without having to jump over to the player <Controls /> component. The same mentality could be applied to your Stats dialog idea.

[ASteinheiser]

@KajiTetsushi I understand the Controls component is getting busy, however I still want to have all the controls in one place. Maybe we just need to break out the specific controls into functions to declutter it?

I can see your point on not wanting to have to remove the keybinding from Controls if Inventory is removed, however I don't believe that will happen. I really just want to be able to go to one place for all the player's controls.

Ideally, the Controls should also have the key listeners for closing dialogs and chests as well, since the player attack/move is "disabled" at that point anyways. This way, we wouldn't need to "disable" the player controls, since the Controls component would dispatch a different action than "attack" or "move", for example.

[Alaricus]

So... are we still doing this?

I'd like to see the inventory get bound to "I" or "B", the only issue I had with the PR was that it made the button unable to be targeted by various screen-readers, etc. Perhaps we can simply unselect it when the inventory gets closed, so that Enter and Spacebar would never trigger it accidentally?

[ASteinheiser]

@Alaricus Good suggestion. I think that would be the best of both worlds.

I would prefer we bind the key to "I", then on the inventory button, change the text from "INVENTORY" -> "[I]NVENTORY" to indicate that you can press I.

[Alaricus]

@ASteinheiser, another question then. Personally I don't use (or test for) the mobile apps, but it appears that the experiences would, at some point, become divergent out of necessity. For instance there is little reason to have the [I] bit show up on a mobile app. Same for the onHover events we discussed earlier. How do you want to go about it? Should there be different repos? Should we fight the divergence and create a unified experience? What's your vision for the long term?

[KajiTetsushi]

Really sorry about the silence, guys. I've just changed jobs recently, trying to adapt to the new work environment and all...

---

@Alaricus: you assumed that I would've gone with my first suggested solution (disabling button focus, because yes, that was my initial recommendation), to which I say: "no", I eventually didn't. I saw it coming, too. Thus, I left the button focus event untouched. If you see this here commit: a7dd028, I've left the button alone. But I've seen you reintroduce my approach in #60, with the extra UI bits in it, so yeah, it's all you now. 👍

---

I really just want to be able to go to one place for all the player's controls.

@ASteinheiser: I understand.

That being said, my first approach was, indeed, to place that keybinding in the controls.js file (see here: a7dd028). I switched to the cf9da09 approach because in my previous experience, putting stuff together in one monolith of a file lead into all kinds of coupling problems. Perhaps that's because the software is big. Perhaps this project is not the case.

However, if this is no concern to you, then, by all means, I've already did it: a7dd028 is my take on solution. I believe @Alaricus has gone with PR #60, which does the same thing (putting the action in controls.js), plus a few bells & whistles on the visuals which I haven't done. I'd gladly defer to that PR.

[Alaricus]

@KajiTetsushi, yes, you are 100% correct, I did assume! Sorry about that! 😄

By the way, I have absolutely no problem deleting my PR if you want to update yours. The only reason I submitted it, is because I saw you being gone for several days and didn't want this issue remain unaddressed. That said, since you are back and this is your idea in the first place, I'll happily step back.

Let me know!

[ASteinheiser]

@Alaricus I definitely don't think that we should have separate repos for the mobile and desktop versions. I honestly think the amount of divergence should be small enough that we can deal with it on a case by case basis.

For example, the game instructions dialog check if it's mobile or desktop to show different controls. Does this sound reasonable to you?

@KajiTetsushi No worries, I'm about to start a new gig next week too, so I will be a little slower soon!

I can see what you're saying about the controls, but I don't envision too many controls needing to be bound.

So, between you guys, should I accept @Alaricus PR or do you want to submit yours @KajiTetsushi ?

[KajiTetsushi]

@Alaricus that's all right, buddy. no hard feelings.

---

Yeah, I can take point again @ASteinheiser @Alaricus, since I was the one who first asked for the fix. I might as well finish what I've started. I'll be available to create a PR again in about 6~8 hours from now. You guys cool with that?