BIRMINGHAM | ITP-JAN 26 | Merve Reis | Sprint 3 | Quote Generator#1043
BIRMINGHAM | ITP-JAN 26 | Merve Reis | Sprint 3 | Quote Generator#1043mervereis wants to merge 2 commits intoCodeYourFuture:mainfrom
Conversation
cjyuan
left a comment
There was a problem hiding this comment.
Code is working good.
- There is some unnecessary code you can remove to keep the code cleaner.
| btn.addEventListener("click", showQuote); | ||
|
|
||
| toggle.addEventListener("change", () => { | ||
| if (toggle.checked) { | ||
| statusEl.textContent = "Auto Play: ON"; | ||
| interval = setInterval(showQuote, 3000); | ||
| } else { | ||
| statusEl.textContent = "Auto Play: OFF"; | ||
| clearInterval(interval); | ||
| interval = null; | ||
| } | ||
| }); | ||
|
|
||
| showQuote(); |
There was a problem hiding this comment.
Placing all the "run on load" code in one place is a good practice.
Would be even better to place the code (and the constants/variables needed locally by these code) inside a function to make it clearer that "this is what runs when the page loads."
For examples,
function setup() {
// code to be executed on page load
}
window.addEventListener('load', setup);or
window.addEventListener('load', function() {
// code to be executed on page load
});There was a problem hiding this comment.
@cjyuan I moved the logic what we run while page load into the setup function then I called it with eventListener as you suggest
| const picked = pickFromArray(quotes); | ||
| console.log(picked); | ||
| quoteEl.textContent = picked.quote; | ||
| authorEl.textContent = "- " + picked.author; |
There was a problem hiding this comment.
Since "- " is part of the presentation logic, it is better to keep it in the CSS or in HTML.
cjyuan
left a comment
There was a problem hiding this comment.
Moving all the code that implements the app into setup() is not what I had in mind; some of those code are just function definitions. What I had in mind was,
... // shared constants/variables declaration
... // function definitions
function setup() {
... // Code that setups the callback functions
showQuote();
}
window.addEventListener("load", setup);
No change required. Organizing code is not an easy task and what I am suggesting here is not the only way. At least you have experienced one way to organize code in a local scope.
Code looks good. Well done.
| } | ||
| function showQuote() { | ||
| const picked = pickFromArray(quotes); | ||
| console.log(picked); |
There was a problem hiding this comment.
This seems like a piece of debugging code. Best practice is to delete all unnecessary code before submitting the code for review.
|
Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it. |
Learners, PR Template
Self checklist
Changelist
Created Quote Generator application.
Updated index.html file, added quote generator functionality and created styles for it.