It looks really good! Here are a few points;
I don't like the arguments in your setLimit
and init
functions, especially because some of them are optional.
I would prefer an Object argument in this case.
function setLimit(limits) {
finalLimit = limits.final;
warnLimit = limits.warn;
errorLimit = limits.error;
}
this.setLimit({
final: 10,
warn: 5,
error: 8
});
This makes it obvious at call time which argument is which.
Your var declarations are inconsistent here:
var finalLimit = null,
warnLimit = null,
errorLimit = null;
var max = 59;
var interval = null;
Pick either to comma separate them or not. Not both.
In a couple of places you use ==
instead of ===
. Generally we stick to ===
for reasons reasons.
You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.
I favour BEM syntax, which would make it look something like this;
<div class="timer">
<div class="timer__labels">
<span id="timer__hours-label">00</span>
: <span id="timer__minutes-label">00</span>
: <span id="timer__seconds-label">00</span>
</div>
<button class="timer__btn js-start-timer">Start</button>
<button class="timer__btn js-end-timer">Stop</button>
<button class="timer__btn js-reset-timer">Reset</button>
</div>
Note I've also removed your onclick
handlers in favour of interaction specific classes. I prefix my classnames with js-
if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.
In your code you then attach the handlers;
document.querySelectorAll('.js-start-timer')[0]
.addEventListener('click', startTimer);
It looks really good! Here are a few points;
I don't like the arguments in your setLimit
and init
functions, especially because some of them are optional.
I would prefer an Object argument in this case.
function setLimit(limits) {
finalLimit = limits.final;
warnLimit = limits.warn;
errorLimit = limits.error;
}
this.setLimit({
final: 10,
warn: 5,
error: 8
});
This makes it obvious at call time which argument is which.
Your var declarations are inconsistent here:
var finalLimit = null,
warnLimit = null,
errorLimit = null;
var max = 59;
var interval = null;
Pick either to comma separate them or not. Not both.
In a couple of places you use ==
instead of ===
. Generally we stick to ===
for reasons.
You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.
I favour BEM syntax, which would make it look something like this;
<div class="timer">
<div class="timer__labels">
<span id="timer__hours-label">00</span>
: <span id="timer__minutes-label">00</span>
: <span id="timer__seconds-label">00</span>
</div>
<button class="timer__btn js-start-timer">Start</button>
<button class="timer__btn js-end-timer">Stop</button>
<button class="timer__btn js-reset-timer">Reset</button>
</div>
Note I've also removed your onclick
handlers in favour of interaction specific classes. I prefix my classnames with js-
if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.
In your code you then attach the handlers;
document.querySelectorAll('.js-start-timer')[0]
.addEventListener('click', startTimer);
It looks really good! Here are a few points;
I don't like the arguments in your setLimit
and init
functions, especially because some of them are optional.
I would prefer an Object argument in this case.
function setLimit(limits) {
finalLimit = limits.final;
warnLimit = limits.warn;
errorLimit = limits.error;
}
this.setLimit({
final: 10,
warn: 5,
error: 8
});
This makes it obvious at call time which argument is which.
Your var declarations are inconsistent here:
var finalLimit = null,
warnLimit = null,
errorLimit = null;
var max = 59;
var interval = null;
Pick either to comma separate them or not. Not both.
In a couple of places you use ==
instead of ===
. Generally we stick to ===
for reasons.
You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.
I favour BEM syntax, which would make it look something like this;
<div class="timer">
<div class="timer__labels">
<span id="timer__hours-label">00</span>
: <span id="timer__minutes-label">00</span>
: <span id="timer__seconds-label">00</span>
</div>
<button class="timer__btn js-start-timer">Start</button>
<button class="timer__btn js-end-timer">Stop</button>
<button class="timer__btn js-reset-timer">Reset</button>
</div>
Note I've also removed your onclick
handlers in favour of interaction specific classes. I prefix my classnames with js-
if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.
In your code you then attach the handlers;
document.querySelectorAll('.js-start-timer')[0]
.addEventListener('click', startTimer);
It looks really good! Here are a few points;
I don't like the arguments in your setLimit
and init
functions, especially because some of them are optional.
I would prefer an Object argument in this case.
function setLimit(limits) {
finalLimit = limits.final;
warnLimit = limits.warn;
errorLimit = limits.error;
}
this.setLimit({
final: 10,
warn: 5,
error: 8
});
This makes it obvious at call time which argument is which.
Your var declarations are inconsistent here:
var finalLimit = null,
warnLimit = null,
errorLimit = null;
var max = 59;
var interval = null;
Pick either to comma separate them or not. Not both.
In a couple of places you use ==
instead of ===
. Generally we stick to ===
for reasons.
You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.
I favour BEM syntax , which would make it look something like this;
<div class="timer">
<div class="timer__labels">
<span id="timer__hours-label">00</span>
: <span id="timer__minutes-label">00</span>
: <span id="timer__seconds-label">00</span>
</div>
<button class="timer__btn js-start-timer">Start</button>
<button class="timer__btn js-end-timer">Stop</button>
<button class="timer__btn js-reset-timer">Reset</button>
</div>
Note I've also removed your onclick
handlers in favour of interaction specific classes. I prefix my classnames with js-
if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.
In your code you then attach the handlers;
document.querySelectorAll('.js-start-timer')[0]
.addEventListener('click', startTimer);
It looks really good! Here are a few points;
I don't like the arguments in your setLimit
and init
functions, especially because some of them are optional.
I would prefer an Object argument in this case.
function setLimit(limits) {
finalLimit = limits.final;
warnLimit = limits.warn;
errorLimit = limits.error;
}
this.setLimit({
final: 10,
warn: 5,
error: 8
});
This makes it obvious at call time which argument is which.
Your var declarations are inconsistent here:
var finalLimit = null,
warnLimit = null,
errorLimit = null;
var max = 59;
var interval = null;
Pick either to comma separate them or not. Not both.
In a couple of places you use ==
instead of ===
. Generally we stick to ===
for reasons.
It looks really good! Here are a few points;
I don't like the arguments in your setLimit
and init
functions, especially because some of them are optional.
I would prefer an Object argument in this case.
function setLimit(limits) {
finalLimit = limits.final;
warnLimit = limits.warn;
errorLimit = limits.error;
}
this.setLimit({
final: 10,
warn: 5,
error: 8
});
This makes it obvious at call time which argument is which.
Your var declarations are inconsistent here:
var finalLimit = null,
warnLimit = null,
errorLimit = null;
var max = 59;
var interval = null;
Pick either to comma separate them or not. Not both.
In a couple of places you use ==
instead of ===
. Generally we stick to ===
for reasons.
You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.
I favour BEM syntax , which would make it look something like this;
<div class="timer">
<div class="timer__labels">
<span id="timer__hours-label">00</span>
: <span id="timer__minutes-label">00</span>
: <span id="timer__seconds-label">00</span>
</div>
<button class="timer__btn js-start-timer">Start</button>
<button class="timer__btn js-end-timer">Stop</button>
<button class="timer__btn js-reset-timer">Reset</button>
</div>
Note I've also removed your onclick
handlers in favour of interaction specific classes. I prefix my classnames with js-
if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.
In your code you then attach the handlers;
document.querySelectorAll('.js-start-timer')[0]
.addEventListener('click', startTimer);
It looks really good! Here are a few points;
I don't like the arguments in your setLimit
and init
functions, especially because some of them are optional.
I would prefer an Object argument in this case.
function setLimit(limits) {
finalLimit = limits.final;
warnLimit = limits.warn;
errorLimit = limits.error;
}
this.setLimit({
final: 10,
warn: 5,
error: 8
});
This makes it obvious at call time which argument is which.
Your var declarations are inconsistent here:
var finalLimit = null,
warnLimit = null,
errorLimit = null;
var max = 59;
var interval = null;
Pick either to comma separate them or not. Not both.
In a couple of places you use ==
instead of ===
. Generally we stick to ===
for reasons.