Podbom01 JS Academy Lesson 01 (Game of Life) #13

Podbi · 2019-09-30T12:13:14Z

No description provided.

* input for board size * input for default state * then Game of Life itself * reset button * Unit Tests * Decomposition to Components

vysinsky

General note: It is always better to use the least amount of stateful components. Because when you start handling state in each component separately, you will get headaches from trying to keep the state synchronized.

The best way for this app would be to have all logic and state in App.js or GameOfLife component and pass state and callbacks to children. This way all child components could be "dumb" components and would just render according to passed props and trigger passed callbacks for correct events.

Nice work, @Podbi!

vysinsky · 2019-10-22T12:57:26Z

src/App.js

@@ -0,0 +1,18 @@
+import React from "react";
+import GameOfLife from "./GameOfLife.js";


Just fyi: .js is not mandatory in imports.

vysinsky · 2019-10-22T13:08:08Z

src/Board.js

+    <table style={tableStyles}>
+      <tbody>
+      {props.boardState.map(function(row, index) {
+        return (<Row row={row} key={index} />)


For this usecase it is probably ok. But generaly, using array index as key is not good practice and can lead to buggy behaviour.

More resources:

https://reactjs.org/docs/lists-and-keys.html#keys

https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318

Thank you for the hint and notes for further reading. 👍

From what I've read looks like that in this case it might not be a big issue to use index, otherwise I could put there some unique ID based on shortid for example.

vysinsky · 2019-10-22T13:20:57Z

src/GameOfLife.js

+  }
+}
+
+export default GameOfLife;


Nicely written component! 👍

Thank you, @vysinsky. It was really interesting to put together this one as there are important changes of state that result in different components to be rendered.
I was surprised how easy it was in the end with render() function and just a simple list of stateful properties.

vysinsky · 2019-10-22T13:22:57Z

src/GameOfLife.test.js

+  fireEvent.change(getByLabelText('input-dimension-cells'), {target: { value: 2}});
+  fireEvent.click(getByText('Vygeneruj herní plán'));
+
+  expect(getAllByLabelText('input-cell')[0]).toBeVisible();


Not a big deal but getAllByLabelText('input-cell') could be assigned to a variable so you would not have to call this function six times.

vysinsky · 2019-10-22T13:24:55Z

src/GameOfLifeBoard.js

+  }
+}
+
+export default GameOfLifeBoard;


What if GameOfLife component will have some logic and changes boardState sent in props?

I'm not sure if I'm following your question, @vysinsky. What if the boardState structure is different? I could use PropSpec to make sure that the initial state is according expectations. Truth is that I have not applied much of "defensive" programming in the pull request.

Are you suggesting something else?

I mean something else. Let's talk about this tomorrow in person :).

vysinsky · 2019-10-22T13:28:23Z

src/GameOfLifeBoard.test.js

+
+  fireEvent.click(getByText('Resetovat'));
+  expect(baseElement).toHaveTextContent('1001001111000101011010001Další generace!Resetovat');
+});


I think you are testing something else than GameOfLifeBoard component here :-). Ideally, next function should be mocked if this should be a unit test.

That sounds like a good idea. Depends on the "Unit". Would you give me a hint for a mocking tool, @vysinsky ?

Jest has it included: https://jestjs.io/docs/en/mock-functions.html

You need to mock the gol module and mock implementation of next function.

I can help you more (give you the code) if you want, let me know :).

And hint: if you change GameOfLifeBoard component to receive next as prop (so you don't import it in the GameOfLifeBoard module) mocking gets much easier 😉

Thank you for these ideas. :-) Dependency injection is a good way.

vysinsky · 2019-10-22T13:29:48Z

src/InputCell.js

+
+  render() {
+    return (
+      <input type="number" name={this.state.name} data-row={this.state.row} data-cell={this.state.cell} min="0" max="1" value={this.state.value} onChange={this.handleOnChange} aria-label="input-cell" />


Are those data attributes just for testing?

Yes. Could I use something more appropriate?

Hm...

you could test the state values. But I am afraid the state is an implementation detail of the component. And react-testing library encourages devs to not test these details (I think it is not even possible to read the state of the component you test).

As you include row and cell in name it should be enough to select by the name value.

vysinsky · 2019-10-22T13:31:10Z

src/InputCell.js

+    this.setState(currentState => {
+      return {
+        ...currentState,
+        value: value


Just a hint: In case object property and variable you are assigning to it have same names, you can use shorthand:

{ ...currentState, value }

Good to know. Thanks!

vysinsky · 2019-10-22T13:31:56Z

src/InputCell.js

+      cell : parseInt(props.cell),
+      value : 0
+    };
+    this.onInputChangeCallback = props.onChange;


Why are you doing this?

So I can use the callback later on, is there a more appropriate way?

You can call props.onChange directly anywhere in the component. The binding should be done in parent component.

vysinsky · 2019-10-22T13:33:46Z

src/InputCell.js

+    });
+
+    if (this.onInputChangeCallback) {
+      this.onInputChangeCallback({


this could be just this.props.onChange({

Also, giving onChange default value of () => {} would remove the need of checking if it is set:

InputCell.defaultProps = { onChange: () => {} };

Cool. I didn't know that it is possible to access this.props!

Yes, props is normal class property, you can access it anywhere in the component (if method is not static ofc)

… used in `GameOfLifeBoard.test.js`.

Podbi · 2019-10-25T06:45:01Z

Thank you for the review, @vysinsky. Great hints and a huge help for me! :-)

I've applied most of your comments.

Regarding the components the idea was to decompose them as much as I can, but the truth is that holding the state might be tricky. The idea to have just one component owning the state sounds right. :-)

vysinsky · 2019-10-25T10:29:20Z

No problem, @Podbi I am happy to help.

Let me know if you want some more personal discussion :-)

Dominik Tilp and others added 4 commits September 13, 2019 12:07

countLiveNeighbours and first rule of GOL

8011c7c

podbom01: JavaScript Academy Lesson 01 (Game of Life)

3e50cae

.gitignore updated

e25754a

Game of Life Updated:

70dca64

* input for board size * input for default state * then Game of Life itself * reset button * Unit Tests * Decomposition to Components

vysinsky reviewed Oct 22, 2019

View reviewed changes

Martin Podborský added 3 commits October 24, 2019 09:40

Unnecessary ".js" removed from imports.

3477d96

Unnecessary ".js" removed from imports.

76a4593

GameOfLife.test.js updated to optimise function call using a variable.

227a5d1

vysinsky mentioned this pull request Oct 24, 2019

Lecture 2/homework #15

Open

Martin Podborský added 2 commits October 25, 2019 08:34

DefaultProps applied and props used for callbacks.

9f55aa1

next function added as a dependency for GameOfLife. Mock function…

4918276

… used in `GameOfLifeBoard.test.js`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Podbom01 JS Academy Lesson 01 (Game of Life) #13

Podbom01 JS Academy Lesson 01 (Game of Life) #13

Podbi commented Sep 30, 2019

vysinsky left a comment

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

Podbi Oct 24, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 22, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 24, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 24, 2019

vysinsky Oct 24, 2019 •

edited

Loading

Podbi Oct 25, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 24, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 24, 2019

vysinsky Oct 22, 2019

Podbi Oct 24, 2019

vysinsky Oct 24, 2019 •

edited

Loading

Podbi commented Oct 25, 2019

vysinsky commented Oct 25, 2019

		@@ -0,0 +1,18 @@
		import React from "react";
		import GameOfLife from "./GameOfLife.js";

Podbom01 JS Academy Lesson 01 (Game of Life) #13

Are you sure you want to change the base?

Podbom01 JS Academy Lesson 01 (Game of Life) #13

Conversation

Podbi commented Sep 30, 2019

vysinsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vysinsky Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vysinsky Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Podbi commented Oct 25, 2019

vysinsky commented Oct 25, 2019

vysinsky Oct 24, 2019 •

edited

Loading

vysinsky Oct 24, 2019 •

edited

Loading