mangamaui / munchkin-level-counter Goto Github PK

I'm sorry if I'm a bit with late with the feedback. I wrote feedback last weekend (slightly more in-depth than this one), but I made the mistake of closing my tab... I hope it's helpful. If you have any questions related to the feedback, ask them! Other questions are welcome too :)

---

You could move the logic inside a React component, which handles the resizing by itself. FaCC (Function as Children Components) and more specifically render props is a nice pattern to abstract logic away in a component and then rendering other components.

I like how to test are written independently from each other. It's good practice that a certain test isn't implicitly covered by other test. That could result in two tests breaking. So good job in this one.

What the reason behind declaring variables as const SOME_VARIABLE? To me that gives the impression those variables are actual constants like: const SOME_VAR = 'someVar', but you are using it like const SOME_VAR = props.someProp`.

It's more a nitpick, but I see action creators in most cases as collaborator (meaning it just calls functions and passes those results in other functions etc), but is not doing any logic itself. In this case you are depending on testing logic in an action creator, while you usually want to separate testing logic from testing collaboration.

Basically what I said above will result in helper function and helper function, which depend on state is a pattern called selectors, which is a pure function with the state as input and the transfformed value as output. These helper functions can be tested separately as a unit.

Good job on try/catch when working with local storage ;)

I can't remember if this is needed or not, if refreshing the page or form or something then sure. But it might be unneeded in many cases.

Some comments could be remove be creating a more descriptive name for your functions or variables. Personally I would go with something like: fireUpdatePlayer, which removes the need of the comment.

Usually with event handler I'd for using onSomeEvent, for example: onPlayerClick and pass the same name around until I actually handle the the player click with something like: onPlayerClick={this.killPlayer}. But that's maybe a personal preference. I tend to avoid using somethingHandler and rather use callToActionSomething. Long names > short names.

I would prefer using let someVariable; > let someVariable = undefined; > let someVariable = void 0; Not everyone know those are equivalent, so it's best not to confuse when working with others.

The component in general is doing a bit too much and depending if you wanted to re-use the functionality of the mouse movement logic, you could extract a component which calculates it's own state and then render wahtever you give it (render props -FaCC)

---

It's a cool project though! It was interesting that React was being used in "game" setting. Very cool!