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

Thanks! #1

Open
coderschoolreview opened this issue Nov 19, 2017 · 0 comments
Open

Thanks! #1

coderschoolreview opened this issue Nov 19, 2017 · 0 comments

Comments

@coderschoolreview
Copy link

Hello Hanoian Champion Toan Ngo.

Good work with the prework, especially the additional features. Interesting implementation on the time feature counting up. One quick suggestion:

Instead of just doing an empty setState, as on https://github.com/hunterxx222/react-prework/blob/master/src/App.js#L29, you should just setState on the current time. https://stackoverflow.com/a/39426527/396324. You can then pass in the current time as a prop to tweet, which I think is much cleaner.

You should also unmount the interval.

https://www.reactenlightenment.com/basic-react-components/6.10.html also has another example of updating using a timer.

Another note is it's not good form to use splice directly on the tweets array in the delete function. Here we're okay, but there are times that you may have a bug where React is confused about whether or not there's a new object. In short: when you mutate an existing object, sometimes React doesn't realize things have changed. The safest way is to create a new "copy" of an object.

Good work! Hope to see more awesome stuff from you, thanks for the cool dynamic timer updating :).

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