London | 25-ITP-Sep | Mariia Serhiienko | Sprint 3 | Quote generator#910
London | 25-ITP-Sep | Mariia Serhiienko | Sprint 3 | Quote generator#910Konvaly wants to merge 10 commits intoCodeYourFuture:mainfrom
Conversation
Konvaly
added
the
Needs Review
| const autoPlayToggle = document.getElementById("auto-play-toggle"); | ||
| const autoPlayStatus = document.getElementById("auto-play-status"); | ||
|
|
||
| newQuoteBtn.addEventListener("click", showRandomQuote); |
There was a problem hiding this comment.
what happens if quoteEl, authorEl, newQuoteBtn, autoPlayToggle, autoPlayStatus or their corresponding ids are not available, missing, not spelled correctly?
There was a problem hiding this comment.
If newQuoteBtn is null, then JavaScript will try to run : "null.addEventListener(...)"
This causes a TypeError (Cannot read properties of null (reading 'addEventListener')), therefore it stops the script, so the whole quote generator will break.
I fixed it with adding helper function which checks if element exist and throws a mistake if not.
| display: inline-block; | ||
| } | ||
|
|
||
| .auto-play-label { |
There was a problem hiding this comment.
is there a way to use the DRY principle between the label and the .auto-play-label? since they both have label ?
There was a problem hiding this comment.
Thanks for the feedback! I updated the CSS so the shared label styles are all in the main label rule.
.auto-play-label now only overrides what’s different (the margin and slightly smaller gap), so it’s more DRY.
Sprint-3/quote-generator/quotes.js
Outdated
| autoPlayToggle.addEventListener("click", () => { | ||
| if (autoPlayToggle.checked) { | ||
| autoPlayStatus.innerText = "auto-play: ON"; | ||
| autoPlayInterval = setInterval(showRandomQuote, 4000); |
There was a problem hiding this comment.
what happens if interval has been set and not cleared before the above line ?
There was a problem hiding this comment.
If the interval isn’t cleared, a new setInterval creates an extra timer, so multiple intervals run at once. I fixed this by clearing the existing interval before starting a new one.
JaypeeLan
added
Review in progress
There was a problem hiding this comment.
Everything works well, but there is an improvement to make. I noticed as I got new quotes, some quotes are quite long, so the height of the container keeps adjusting, and I have to move my cursor everytime that happens. So, what you can do is figure out how you can keep the height of the container, so the next button stays in its position. Maybe use ellipsis and a "show more" button if the quote is too long. Be creative.
When you have cards that displays a list of content, you have to keep the height uniform.
There was a problem hiding this comment.
Hi @JaypeeLan, thanks for pointing that out! I noticed the layout shifting with longer quotes too. I updated the card so it keeps a consistent height, which stops the button from moving around. For quotes that are really long, I added an ellipsis and a “Show more / Show less” toggle, truncated to 2 lines. This way the layout stays steady, but users can still read the full quote if they want to.
JaypeeLan
added
Reviewed
… direct DOM lookups
Konvaly
added
Needs Review
|
Hi @takanocap , @JaypeeLan ! Could you check my PR fixed according to your reviews, please? |
JaypeeLan
added
Complete
|
Hi @JaypeeLan , @takanocap ! Thank you for reviewing my PR! |
4 participants
Self checklist
This PR completes and extends the original Quote Generator task.
It implements all required functionality (displaying random quotes, updating on button click) and adds an enhanced auto-play mode.
Additionally, the UI has been redesigned.
Key updates:
Implemented the core Quote Generator logic (random quote on load + on button click)
Added auto-play toggle to generate new quotes automatically
Added interval-based auto-generation with start/stop control
Displayed “auto-play: ON/OFF” status dynamically
Improved styling in CSS-file