Manchester| 25-ITP-Sep| Fithi Teklom| Sprint 3 | Quote Generator App#915
Manchester| 25-ITP-Sep| Fithi Teklom| Sprint 3 | Quote Generator App#915Fithi-Teklom wants to merge 8 commits intoCodeYourFuture:mainfrom
Conversation
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (Sprint-3) doesn't match expected format (example: 'Sprint 2', without quotes) If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
Fithi-Teklom
changed the title
Fithi-Teklom
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code looks good.
I just have some suggestions.
| let autoPlayInterval = null; | ||
|
|
||
| const toggle = document.getElementById("auto-play-toggle"); | ||
| const statusText = document.getElementById("auto-status"); | ||
|
|
||
| toggle.addEventListener("change", () => { | ||
| if (toggle.checked) { | ||
| statusText.textContent = "auto-play:ON"; | ||
|
|
||
| // Change every 5 seconds (easy for testing) | ||
| autoPlayInterval = setInterval(() => { | ||
| pickQuoteAndAuthor(); | ||
| }, 5000); | ||
|
|
||
| pickQuoteAndAuthor(); // Change immediately | ||
| } else { | ||
| statusText.textContent = "auto-play:OFF"; | ||
| clearInterval(autoPlayInterval); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Could consider wrapping all the code and variables needed to support auto-play inside a function as:
function setupAutoPlay() {
// JavaScript support closure. So even after this function call returns, the
// inner function can still access these variables.
let autoPlayInterval = null;
const toggle = document.getElementById("auto-play-toggle");
const statusText = document.getElementById("auto-status");
toggle.addEventListener("change", () => {
...
});
}and then call the function in the onload event listener as
window.addEventListener("load", () => {
button.addEventListener("click", displayRandomQuote);
// By placing the setup code in a separate function, it makes finding out "what operations are
// performed once on page load" easier.
setupAutoPlay();
displayRandomQuote();
});| const quoteEl = document.getElementById("quote"); | ||
| const authorEl = document.getElementById("author"); | ||
| const button = document.getElementById("new-quote"); | ||
|
|
||
| // Generate a random quote using Math.random (required for tests) | ||
| function getRandomQuote() { | ||
| const index = Math.floor(Math.random() * quotes.length); | ||
| return quotes[index]; | ||
| } | ||
|
|
||
| // Put the quote + author into the DOM | ||
| function displayRandomQuote() { | ||
| const q = getRandomQuote(); | ||
| quoteEl.textContent = q.quote; | ||
| authorEl.textContent = q.author; | ||
| } |
There was a problem hiding this comment.
Could consider declaring top-level variables and defining functions outside of the onload event listener. Doing so could make finding out "what operations are performed once on page load" easier.
cjyuan
added
Complete
|
You missed leaving a changelist section in the PR description. |
3 participants
Self checklist