Please Review my React Pomodoro Clock

Please Review my React Pomodoro Clock
0.0 0

#1

Please review my Pomodoro Clock! It’s my first app built in React.

You can find my source code here.

I had some issues with SVG filters making my app slow in Firefox mobile that I couldn’t resolve.


#2

Hi @ellereeeee :slight_smile: ,

README (GitHub)
  • Is a really good readme, but I couldn’t find a link to the pomodoro:
    (There is one in the description(?) of the project in Github, but not in the readme)
pomodoro-clock/src/components/PomodoroTimer.jsx
  • Is there any reason not to use the second form of setState() ?:
handleIncrementTime = () => {
    this.setState({ state: (this.state.time += 300000) });
  };
  handleDecrementTime = () => {
    if (this.state.time > 300000) {
      this.setState({ time: (this.state.time -= 300000) });
    }
  };

Reacts Documentation:

https://reactjs.org/docs/state-and-lifecycle.html

this.setState((prevState, props) => ({
  counter: prevState.counter + props.increment
}));

Cheers and happy coding :slight_smile:


#3

Wow thank you Diego :slight_smile:!


#4

Hey @erretres,

I looked at the documentation for the second form of setState and it says you’re supposed to use it when you’re updating the state with props, right? It looks like it solves an asynchronicity issue between the two.

In my case I’m not updating state with props; I’m just adding or subtracting 5 minutes to or from the state. Is there a way I could use the second form of setState?


#5

Hi @ellereeeee,

I cloned the repo and I am getting warnings:

Compiled with warnings.

./src/components/Info.jsx
  Line 16:  Using target="_blank" without rel="noopener noreferrer" is a security risk: see https://mathiasbynens.github.io/rel-noopener  react/jsx-no-target-blank

./src/components/PomodoroTimer.jsx
  Line 17:   Do not mutate state directly. Use setState()  react/no-direct-mutation-state
  Line 21:   Do not mutate state directly. Use setState()  react/no-direct-mutation-state
  Line 33:   Expected '===' and instead saw '=='           eqeqeq
  Line 52:   Expected '===' and instead saw '=='           eqeqeq
  Line 78:   Expected '===' and instead saw '=='           eqeqeq
  Line 109:  Expected '===' and instead saw '=='           eqeqeq

Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.   

The important part is:

  Line 17:   Do not mutate state directly. Use setState()  react/no-direct-mutation-state

Changes

  1. I added a constructor to PomodoroTimer.jsx

  2. Now state is this.state and is inside the constructor

class PomodoroTimer extends Component {
 constructor(props) {
  super(props); 
  this.state = {
    toggleInfo: false,
    timerActive: false,
    time: 1500000,
    timerType: "Pomodoro",
    offsetModifier: 1
  }; 
 } 
  1. I modified the handleIncrementTime function:
handleIncrementTime = () =>  this.setState((prevState) => ({ time: prevState.time += 300000 }));

Result

  • There is no warning for handleIncrementTime function
Compiled with warnings.

./src/components/Info.jsx
  Line 16:  Using target="_blank" without rel="noopener noreferrer" is a security risk: see https://mathiasbynens.github.io/rel-noopener  react/jsx-no-target-blank

./src/components/PomodoroTimer.jsx
  Line 25:   Do not mutate state directly. Use setState()  react/no-direct-mutation-state
  Line 37:   Expected '===' and instead saw '=='           eqeqeq
  Line 56:   Expected '===' and instead saw '=='           eqeqeq
  Line 82:   Expected '===' and instead saw '=='           eqeqeq
  Line 113:  Expected '===' and instead saw '=='           eqeqeq

Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.

suggestions

  1. Refactor handleDecrementTime (using functional setState [second form])

  2. Refactor handleIncrementTime, handleDecrementTime, handleStateTime:
    Following the pattern: “event handler method on the class”

Reactjs documentation:

When you define a component using an ES6 class, a common pattern is for an event handler to be a method on the class. For example, this Toggle component renders a button that lets the user toggle between “ON” and “OFF” states:

class Toggle extends React.Component {
  constructor(props) {
    super(props);
    this.state = {isToggleOn: true};

    // This binding is necessary to make `this` work in the callback
    this.handleClick = this.handleClick.bind(this);
  }

  handleClick() {
    this.setState(prevState => ({
      isToggleOn: !prevState.isToggleOn
    }));
  }

Cheers and happy coding :slight_smile:

note:
I suspect that the problem with Firefox mobile can be because the state is not correctly updated.


#6

Thanks again Diego! I get it now; it’s all about not modifying state directly.

I solved the issue using the second form of setState like so:

  handleIncrementTime = () =>
    this.setState(prevState => ({ time: (prevState.time += 300000) }));
  handleDecrementTime = () => {
    if (this.state.time > 300000) {
      this.setState(prevState => ({ time: (prevState.time -= 300000) }));
    }
  };

I have a couple questions regarding your other suggestions though. Isn’t making a constructor and putting state inside a preference in this situation? I’m using public class fields to declare state outside the constructor and inside the class body and everything seems to work fine.

Also, aren’t I already using the “event handler method on the class” for my handlers? I understand that as meaning just declaring the handlers in the class body.

Unfortunately the laggy issue in Firefox mobile remains, but I’m pretty sure it has to do with poor SVG performance in that browser. Removing the SVG filter property from the radial timer makes my app as snappy as in other browsers.


#7

Hi @ellereeeee,

I didn’t know about the public class fields, thanks for explain it :slightly_smiling_face: .

I see, nothing to do then.

Cheers and happy coding :slight_smile: