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

[Feature] - Add Timer #33

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

[Feature] - Add Timer #33

wants to merge 9 commits into from

Conversation

AnaCr
Copy link
Collaborator

@AnaCr AnaCr commented Jul 30, 2020

Description

Add timer
image

Fixes

Fix #6

@AnaCr AnaCr requested a review from ivnflpz July 30, 2020 02:50
@AnaCr AnaCr changed the title Timer [Feature] - Add Timer Jul 30, 2020
Copy link
Owner

@ivnflpz ivnflpz left a comment

Choose a reason for hiding this comment

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

Good stuff! Left some minor comments.

Comment on lines 20 to 21
const [minutes, setMintues] = React.useState(0);
const [seconds, setSeconds] = React.useState(-5);
Copy link
Owner

@ivnflpz ivnflpz Jul 30, 2020

Choose a reason for hiding this comment

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

I like prefixing state variables with s to quickly identify them when reading through the code so these would become sMinutes and sSeconds.

Comment on lines 58 to 59
{minutes < 10 ? 0 : ''}
{minutes}:{seconds < 10 ? 0 : ''}
Copy link
Owner

Choose a reason for hiding this comment

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

you can use something like padStart from lodash to handle adding these zeros for you.

<h1>
{minutes < 10 ? 0 : ''}
{minutes}:{seconds < 10 ? 0 : ''}
{seconds < 0 ? Math.abs(seconds) : seconds}
Copy link
Owner

Choose a reason for hiding this comment

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

Since lookup time will always be at a maximum 15 seconds, could we only show seconds there. eg. when seconds state is negative we only show seconds and once its positive, it goes to minutes:seconds.

.eslintrc.js Outdated
"prettier/prettier": ["error", {
"endOfLine":"auto"
}],
'no-console': 'off',
Copy link
Owner

Choose a reason for hiding this comment

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

I think we might be able to get rid of this line. I'm not seeing any consoles that would require turning it off.

@pull-assistant
Copy link

Score: 0.99

Best reviewed: commit by commit


Optimal code review plan (1 warning, 2 commits squashed)

     Add timer

     Merge branch 'master' into timer

fix: lint issues

.eslintrc.js 50% changes removed in Remove eslint 'no-co...

     refactor: use correct .env file

     Remove eslint 'no-console' rule

Rename state variabl... ... Use padStart

Squashed 2 commits:

     Distinguish countdown and timer

     Merge branch 'timer' of https://github.com/ivnflpz/trainer-cube into t...

Powered by Pull Assistant. Last update 5ca6ecf ... 7e1121b. Read the comment docs.

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.

Timer: create timer
2 participants