Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React Feedback #1

Open
venikx opened this issue Aug 30, 2018 · 0 comments
Open

React Feedback #1

venikx opened this issue Aug 30, 2018 · 0 comments

Comments

@venikx
Copy link

venikx commented Aug 30, 2018

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 :)


https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/index.js#L32
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.

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/tests/core.test.js#L53
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`.

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/APP/core/actions.js#L90
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.

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/APP/core/actions.js#L231
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.

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/APP/core/localstorage.js#L5
Good job on try/catch when working with local storage ;)

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/APP/levelCounter/index.js#L25
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.

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/APP/views/defeatScreen.js#L100
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.

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/APP/views/overviewScreen.js#L36
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.

https://github.com/Mangamaui/Munchkin-level-counter/blob/0b28df6156bb77b7f78af85776a5065c448b2ace/src/APP/views/overviewScreen.js#L14
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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant