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

ToCode Solution: תרגול מחזור חיים #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

advalicht
Copy link

No description provided.

Copy link
Collaborator

@ynonp ynonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good and quick work.

I think I noticed a logic error in the implementation - In that the rate calculation is discrete. What I mean by that is consider I clicked 10 times within the second half of a second.
In the beginning of the next second my rate is zero, even though I just clicked like crazy and it sky rocketed just a few milliseconds earlier.

It would be nice to show the rate in a "window" of "the last second", in every point in time

}


componentWillMount = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use componentWillMount. Ever. Everything you want to write here is better written in the constructor or in componentDidMount.
(If it uses the DOM it should be in componentDidMount. Otherwise in constructor)



componentWillMount = () => {
this.timer = setInterval(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should certainly be in componentDidMount

<button onClick={this.count} >Click Fast</button>
<p>CPS rate: {this.state.rate}</p>
<p>{this.state.rate > 4 ? "not so fast..." : "faster"}</p>
<Rect color={this.state.rate > 4 ? "green" : "red"}/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I would prefer to pass to Rect the current rate and let it decide on the color. But that may be a matter of taste.

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

Successfully merging this pull request may close these issues.

2 participants