Skip to content

Conversation

@M-Abdoon
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

Quote generator app exercise done.

Questions

No questions so far, thank you.

@M-Abdoon M-Abdoon added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 15, 2025
@tee4tao tee4tao 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 22, 2025
Comment on lines 8 to 9
quoteText["textContent"] = `“${randomQuote.quote}”`;
authorText["textContent"] = `- ${randomQuote.author}`;
Copy link

Choose a reason for hiding this comment

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

quoteText["textContent"] works but there is a cleaner and a more conventional way to set textContent. Can you update it to the more standard approach?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I just updated the code. I used the standard way. which is quoteText.textContent instead of quoteText["textContent"] .

showRandomQuote();
}

window.onload = setup;
Copy link

Choose a reason for hiding this comment

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

Your code is clean and well-organized, and wrapping everything inside a setup() function is a good way to avoid global variables. However, window.onload waits for images, fonts, and external files to finish loading, which makes it slower than necessary. Can you think of a different event or method that would allow your setup code to run as soon as the HTML is ready?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I used document.addEventListener("DOMContentLoaded", setup); insteadinstead so the setup runs as soon as the HTML is ready.

Copy link

Choose a reason for hiding this comment

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

Good. Well-done!

@tee4tao tee4tao added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Nov 22, 2025
@M-Abdoon M-Abdoon added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 23, 2025
@M-Abdoon
Copy link
Author

The changes have been made. Thank you.

@tee4tao tee4tao added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Nov 24, 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants