-
-
Notifications
You must be signed in to change notification settings - Fork 147
NW | ITP-May-25 | Geraldine Edwards | Module-Data-Groups | Sprint-3 | Quote Generator App #586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
NW | ITP-May-25 | Geraldine Edwards | Module-Data-Groups | Sprint-3 | Quote Generator App #586
Conversation
…on page load, then generate a new random quote when button is clicked
Hi @ednut, thank you for reviewing my PR. :) Can you kindly change the label to "complete" if you are satisfied with my code please? (I am happy to have any feedback too, if you feel there are any tweaks I need to make?) Thanks again! |
Hi @ednut - I'm not sure you're signed up as a volunteer at CodeYourFuture at the moment - if you are, can you drop me a DM on Slack? (I'm Daniel Wagner-Hall) to update our records and get you set up? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generally looking good, but I've left a couple of things to think about :)
@@ -491,3 +517,4 @@ const quotes = [ | |||
]; | |||
|
|||
// call pickFromArray with the quotes array to check you get a random quote | |||
const randomQuote = pickFromArray(quotes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this variable do, and how is it used? What would break if you removed it?
document.querySelector("#quote").innerText = randomQuote.quote; | ||
document.querySelector("#author").innerText = randomQuote.author; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "set both elements to show the quote details" behaviour is repeated twice in your app - if we needed to change it in the future, we'd need to change both locations. Can you think of a way to avoid this?
Learners, PR Template
Self checklist
Changelist
Add JS functionality and styling to Quote Generator App.
Questions
Ask any questions you have for your reviewer.