Yoman Project 1 PTBC5 by Flashrob · Pull Request #10 · rocketacademy/project1-bootcamp

Flashrob · 2022-10-26T16:41:36Z

No description provided.

…ound music

…ce image animation

Flashrob · 2022-10-26T16:43:29Z

src/App.js

+    const { name, value } = event.target;
+    event.preventDefault();
+    this.state.audio.play(); // start playing background music
+    this.state.audio.loop = true;


We never want to reassign state like so, but always use the setState function. By doing this here, you will run into trouble with missing rerenders and possibly out-of-sync state.

Flashrob · 2022-10-26T16:45:23Z

src/App.js

+        this.state.diceOrder === 1
+          ? this.state.playerDiceRolls[0] * 10 + this.state.playerDiceRolls[1]
+          : this.state.playerDiceRolls[1] * 10 + this.state.playerDiceRolls[0];
+      const newScores = this.state.userScores.slice(); //copy the array


We could just do this

Suggested change

const newScores = this.state.userScores.slice(); //copy the array

const newScores = [...this.state.userScores]

Flashrob · 2022-10-26T16:46:35Z

src/App.js

+
+  resetGame = () => {
+    this.setState({
+      // Reset the whole game to let user play again


I think ideally we define the defaultValues from 36 - 48 here. We could create a defaultValues object, and use it in line 36, and here.

Suggested change

// Reset the whole game to let user play again

this.setState(defaultState)

Flashrob · 2022-10-26T16:47:00Z

src/App.js

+
+  toggleMusic = (event) => {
+    event.preventDefault();
+    let isMusicOn = this.state.isMusicOn;


I think no need to define variable for state, which is a variable already.

Flashrob · 2022-10-26T16:48:27Z

src/App.js

+    const gameStarted = this.state.gameStarted;
+    const userRollDice = this.state.userRollDice;
+    const userOrderDice = this.state.userOrderDice;
+    const playerDiceRolls = this.state.playerDiceRolls;
+    const userScores = this.state.userScores;


Same here, no need to create new variables. If you absolutely want to, use destructuring for nicer syntax!

Suggested change

const gameStarted = this.state.gameStarted;

const userRollDice = this.state.userRollDice;

const userOrderDice = this.state.userOrderDice;

const playerDiceRolls = this.state.playerDiceRolls;

const userScores = this.state.userScores;

const { gameStatrted, userRollDice, userOrderDice, playerDiceRolls, userScores } = this.state;

Flashrob · 2022-10-26T16:49:05Z

src/App.js

+      this.state.currentPlayer,
+      this.state.numberOfRounds,
+      this.state.numberOfPlayers,
+      this.state.userOrderDice


since you defined userOrderDice right above, let's use that variable at least

Flashrob · 2022-10-26T16:50:18Z

src/DiceImage.js

+import five from "./assets/five.png";
+import six from "./assets/six.png";
+
+export const DiceImage = ({ roll }) => {


Very nice component!

Flashrob · 2022-10-26T16:52:55Z

src/DiceImage.js

+  switch (roll) {
+    case 1:
+      return <img className="dice-image" src={one} alt="1" />;
+    case 2:
+      return <img className="dice-image" src={two} alt="2" />;
+    case 3:
+      return <img className="dice-image" src={three} alt="3" />;
+    case 4:
+      return <img className="dice-image" src={four} alt="4" />;
+    case 5:
+      return <img className="dice-image" src={five} alt="5" />;
+    default:
+      return <img className="dice-image" src={six} alt="6" />;


We could improve this like so:

First name your images as numbers, instead of letters.
"./assets/1.png"; etc.

then like so:

Suggested change

switch (roll) {

case 1:

return <img className="dice-image" src={one} alt="1" />;

case 2:

return <img className="dice-image" src={two} alt="2" />;

case 3:

return <img className="dice-image" src={three} alt="3" />;

case 4:

return <img className="dice-image" src={four} alt="4" />;

case 5:

return <img className="dice-image" src={five} alt="5" />;

default:

return <img className="dice-image" src={six} alt="6" />;

return <img className="dice-image" src={`./assets/${roll}.png`} alt={roll} />;

Flashrob · 2022-10-26T16:54:23Z

src/Instruction.js

+  if (userRollDice) {
+    return (
+      <img
+        src="https://www.animatedimages.org/data/media/710/animated-dice-image-0085.gif"
+        border="0"
+        alt="animated-dice-image-0085"
+        className="animated-gif"
+      />
+    );
+  } else {
+    return <p>Click [Continue] button...</p>;
+  }


Suggested change

if (userRollDice) {

return (

<img

src="https://www.animatedimages.org/data/media/710/animated-dice-image-0085.gif"

border="0"

alt="animated-dice-image-0085"

className="animated-gif"

/>

);

} else {

return <p>Click [Continue] button...</p>;

}

return userRollDice ? <img

src="https://www.animatedimages.org/data/media/710/animated-dice-image-0085.gif"

border="0"

alt="animated-dice-image-0085"

className="animated-gif"

/> : <p>Click [Continue] button...</p>

or get rid of the else statement for better readability

Suggested change

if (userRollDice) {

return (

<img

src="https://www.animatedimages.org/data/media/710/animated-dice-image-0085.gif"

border="0"

alt="animated-dice-image-0085"

className="animated-gif"

/>

);

} else {

return <p>Click [Continue] button...</p>;

}

if (userRollDice) return (

<img

src="https://www.animatedimages.org/data/media/710/animated-dice-image-0085.gif"

border="0"

alt="animated-dice-image-0085"

className="animated-gif"

/>

);

return <p>Click [Continue] button...</p>;

Flashrob · 2022-10-26T16:56:22Z

src/PlayForm.js

+  isLastPlayer,
+  isMusicOn,
+}) => {
+  const captionMusic = isMusicOn ? "Music OFF" : "Music ON";


captionMusic sounds like an action, a function. musicCaption might be better

Flashrob · 2022-10-26T16:57:31Z

@yomansg please see these comments

Flashrob · 2022-11-02T11:51:54Z

src/App.js

+        <div className="Main-body height-fixed">
+          {/* Constantly show the players' scores on a scoreboard*/}
+          {gameStarted && <h4>Leaderboard 🏆</h4>}
+          {gameStarted && <hr />}
+          {gameStarted && <LeaderBoard userScores={userScores} />}
+        </div>


Suggested change

<div className="Main-body height-fixed">

{/* Constantly show the players' scores on a scoreboard*/}

{gameStarted && <h4>Leaderboard 🏆</h4>}

{gameStarted && <hr />}

{gameStarted && <LeaderBoard userScores={userScores} />}

</div>

{ gameStarted && (<div className="Main-body height-fixed">

{/* Constantly show the players' scores on a scoreboard*/}

<h4>Leaderboard 🏆</h4>

<hr />

<LeaderBoard userScores={userScores} />

</div>)}

Flashrob · 2022-11-02T12:30:30Z

src/App.js

+    this.state.audio.play(); // start playing background music
+    this.state.audio.loop = true;


Suggested change

this.state.audio.play(); // start playing background music

this.state.audio.loop = true;

const audio = this.state.audio;

audio.play();

audio.loop = true;

Flashrob · 2022-11-02T12:32:19Z

src/App.js

+      [name]: value,
+      userScores: Array(this.state.numberOfPlayers).fill(0),
+      gameStarted: true,
+      isMusicOn: true,


Suggested change

isMusicOn: true,

isMusicOn: true,

audio: {

...this.state.audio,

loop: true

}

Flashrob · 2022-11-02T12:56:56Z

src/App.js

+    label: "Game music",
+  },
+];



Suggested change

const defaultState = {

gameStarted: false,

numberOfPlayers: 2, // form input state

numberOfRounds: 3,

userRollDice: false,

userOrderDice: false,

userScores: [],

currentPlayer: 0,

currentRound: 0,

playerDiceRolls: [0, 0],

currentPlayerScore: 0,

diceOrder: 1,

audio: new Audio(audioClips[2].sound), // set background music

isMusicOn: false, // background music is initially OFF

}

Flashrob · 2022-11-02T12:57:17Z

src/App.js

+    this.state = {
+      gameStarted: false,
+      numberOfPlayers: 2, // form input state
+      numberOfRounds: 3,
+      userRollDice: false,
+      userOrderDice: false,
+      userScores: [],
+      currentPlayer: 0,
+      currentRound: 0,
+      playerDiceRolls: [0, 0],
+      currentPlayerScore: 0,
+      diceOrder: 1,
+      audio: new Audio(audioClips[2].sound), // set background music
+      isMusicOn: false, // background music is initially OFF
+    };


Suggested change

this.state = {

gameStarted: false,

numberOfPlayers: 2, // form input state

numberOfRounds: 3,

userRollDice: false,

userOrderDice: false,

userScores: [],

currentPlayer: 0,

currentRound: 0,

playerDiceRolls: [0, 0],

currentPlayerScore: 0,

diceOrder: 1,

audio: new Audio(audioClips[2].sound), // set background music

isMusicOn: false, // background music is initially OFF

};

this.state = defaultState;

Flashrob · 2022-11-02T12:59:55Z

src/App.js

+    if (isMusicOn) {
+      this.state.audio.pause(); // Pause music if it is playing
+    } else {
+      this.state.audio.play(); // Play music if it is paused
+    }


Suggested change

if (isMusicOn) {

this.state.audio.pause(); // Pause music if it is playing

} else {

this.state.audio.play(); // Play music if it is paused

}

isMusicOn ? this.state.audio.pause() : this.state.audio.play()

yomansg added 7 commits October 11, 2022 22:20

Basic version of Beat That

e0dfec2

Added dice image and successfully determined game winner

cea0ddd

Added sound and some styling effects

ecaa308

Various styling effects - for buttons, radio buttons and screen height

6210bb6

Used CSS to implement animation for dice rolling images; added backgr…

82fb29c

…ound music

Modified styling slightly for more seamless transition of dice images

90a7409

Add button to toggle music ON/OFF; logo is bigger; more frames for di…

27f147a

…ce image animation

Flashrob commented Oct 26, 2022

View reviewed changes

Flashrob changed the title ~~Code review PR~~ Yoman Project 1 PTBC5 Nov 2, 2022

Flashrob commented Nov 2, 2022

View reviewed changes

Amended code for improved readability after Code Review

b70b2c1

charlesleezhaoyi added a commit to charlesleezhaoyi/project1-bootcamp that referenced this pull request Nov 18, 2023

Commit rocketacademy#10 - Deploy to Github Pages

caaf739

charlesleezhaoyi added a commit to charlesleezhaoyi/project1-bootcamp that referenced this pull request Nov 18, 2023

Commit rocketacademy#10 - Deploy to Github Pages

94484bb

	const newScores = this.state.userScores.slice(); //copy the array
	const newScores = [...this.state.userScores]

	// Reset the whole game to let user play again
	this.setState(defaultState)

		this.state.audio.play(); // start playing background music
		this.state.audio.loop = true;

-      isMusicOn: true,
+      isMusicOn: true,
+      audio: {
+         ...this.state.audio,
+         loop: true
+      }

Conversation

Flashrob commented Oct 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flashrob commented Oct 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants