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.
Sprint-3/quote-generator/quotes.js
Outdated
| 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
Sprint-3/quote-generator/quotes.js
Outdated
| 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.
Learners, PR Template
Self checklist
Changelist
Created Quote Generator application.
Updated index.html file, added quote generator functionality and created styles for it.