Skip to content

Conversation

@Alaa-Tagi
Copy link

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

I have completed all the tasks that required in this sprint.

Questions

No questions.

@Alaa-Tagi Alaa-Tagi added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Groups The name of the module. labels Nov 26, 2025
Copy link

@jubelAhmed jubelAhmed left a comment

Choose a reason for hiding this comment

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

During initial load, if the JS script run before the DOM is ready?
Now fix all the comments

<p id="author"></p>
<button type="button" id="new-quote">New quote</button>
<h1>Quote Generator</h1>
<p id="quote-container"></p>

Choose a reason for hiding this comment

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

The quote-container element is not needed here

return choices[Math.floor(Math.random() * choices.length)];
}
function showRandomQuote() {
const randomQuote = pickFromArray(quotes);

Choose a reason for hiding this comment

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

You can add condition after that is randomQuote is available or not, if not , then return

Comment on lines +24 to +25
const quoteText = document.getElementById("quote-text");
const quoteAuthor = document.getElementById("quote-author");

Choose a reason for hiding this comment

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

During initial load, if the script runs before the DOM is ready, getElementById may return null for these elements. This will cause a TypeError: Cannot set property 'innerText' of null when trying to access quoteText.innerText or quoteAuthor.innerText.

Comment on lines +30 to +31
window.onload = showRandomQuote;
document.getElementById("new-quote-button").addEventListener("click", showRandomQuote);

Choose a reason for hiding this comment

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

The button element might not exist during first load when the JS script runs before the DOM element is ready. If new-quote-button doesn't exist when trying to add the event listener, it will result in:

TypeError: Cannot read property 'addEventListener' of null

Please wrap the event listener setup inside window.onload and add a null check:

window.onload = function() {
showRandomQuote();
const button = document.getElementById("new-quote-button");
if (button) {
button.addEventListener("click", showRandomQuote);
}
};

This ensures the DOM is fully loaded before accessing elements and prevents the TypeError.

@A-O-Emmanuel A-O-Emmanuel added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 30, 2025
Copy link

@A-O-Emmanuel A-O-Emmanuel left a comment

Choose a reason for hiding this comment

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

Your quote generator app works as intended, but please go through this article 10 Code comments best practices for developers.

@A-O-Emmanuel A-O-Emmanuel added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Nov 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Module-Data-Groups The name of the module. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants