justindwyer6 / rolling-realms

A fan-made, web-based version of Stonemaier Games' roll-and-write game.
https://rolling-realms.netlify.app
18 stars 3 forks source link

Feat/d6 component #68

Closed justindwyer6 closed 2 years ago

justindwyer6 commented 2 years ago

This PR adds a new component that provides an input element with all of the (surprisingly complex) logic necessary to limit its value from 1-6. Any character other than 1-6 will not be accepted. If you type a second character, it will just overwrite the first one, so you can easily update the number without needing to hit backspace.

I also replaced the sketchy one-off inputs that existed throughout the app with this new component, which helped clean up a few files.

abrad45 commented 2 years ago

Summarizing our discussion on Discord, you said:

there's just no way around preventInvalidKeyStrokes that I can find. HTML seems to be weirdly stubborn about not letting React hijack those characters any other way. I tried using parseInt but React never even receives the value. Somehow HTML takes over when you enter one of those blacklist characters and React loses control of the input. It's super weird.

Maybe we should start with this: What are the requirements of DSixInput? If I understand correctly:

  1. Allows you to enter a value from 1-6 (call this, x).
  2. while the field already has a value:
    • Entering a new valid (that is, [1,6]) value (call this, y) will replace x with y.
    • Entering a new invalid value (anything else; again, y) will discard y and keep x.

Is there anything else? This should be the first step to determining what the best way to ensure our solution encompasses those needs

justindwyer6 commented 2 years ago

Summarizing our discussion on Discord, you said:

there's just no way around preventInvalidKeyStrokes that I can find. HTML seems to be weirdly stubborn about not letting React hijack those characters any other way. I tried using parseInt but React never even receives the value. Somehow HTML takes over when you enter one of those blacklist characters and React loses control of the input. It's super weird.

Maybe we should start with this: What are the requirements of DSixInput? If I understand correctly:

  1. Allows you to enter a value from 1-6 (call this, x).
  2. while the field already has a value:

    • Entering a new valid (that is, [1,6]) value (call this, y) will replace x with y.
    • Entering a new invalid value (anything else; again, y) will discard y and keep x.

Is there anything else? This should be the first step to determining what the best way to ensure our solution encompasses those needs

@abrad45 Yep, those are essentially the requirements. Some other requirements are that:

  1. An empty value should be allowed
  2. Deleting the character should be allowed
  3. Arrow keys to increment/decrement the value should be allowed
  4. Pasting anything other than 1-6 should not be allowed
abrad45 commented 2 years ago

It seems like the hardest requirement is that non-numerical input is ignored while values can be stepped through. This is a disagreement in the implementation of input[type=number]: See https://bugzilla.mozilla.org/show_bug.cgi?id=1398528

This codesandbox is as close as I can get in as succinct a way I can, but if you type w, nothing stops you. This seems to be specifically because of the following two props:

https://codesandbox.io/s/vigorous-frog-wxg4i?file=/src/DSixInput.jsx

Two lines have //@justin at the end of them. You may uncomment those for some convenient logging.


I raised this in the first place because my role at work where I am an engineering manager and not a developer is to look for code that is fragile or may be difficult to maintain and help everyone come to an agreement about what the best course of action is. Could we build this in a better way? Are all of these requirements truly requirements? I personally would prioritize relaxing a requirement or two to make the code simpler, but you are the manager of this project, not me, and realistically, we won't need to touch this stuff often.

TLDR: I have a better appreciation for the problem at hand and I'm comfortable with you merging your code as is. I do like the useInput pattern I've got in my codesandbox and encourage you to steal it or modify it however you like, but I don't want to hold up the merge here. Proceed as you wish. I support it either way :)