London | 26-ITP-May | Dagim Daniel | Sprint 3 | Quote-generator - #1333
London | 26-ITP-May | Dagim Daniel | Sprint 3 | Quote-generator#1333Dagim-Daniel wants to merge 2 commits into
Conversation
Dagim-Daniel
added
Needs Review
| const areaOfQuote = document.querySelector("h1"); | ||
| const shufflebutton = document.querySelector("#new-quote"); | ||
| const AreaOfQuoteBy = document.querySelector("h3"); |
There was a problem hiding this comment.
-
Better to use meaningful id or class names instead of relying on
h1andh3. -
Is your text editor showing
AreaOfQuoteByand the other variable names in different colors. If so, do you know why?
There was a problem hiding this comment.
my editor showing me with the same colors to the others
There was a problem hiding this comment.
Are you using VS Code? On GitHub, this is how they look like:
const shufflebutton = document.querySelector("#new-quote");
const AreaOfQuoteBy = document.querySelector("h3");What I wanted to point out is that the naming convention for variables in JS is lower camel case.
| areaOfQuote.textContent = `" ${quot} "`; | ||
| AreaOfQuoteBy.textContent = `- ${Author}`; |
There was a problem hiding this comment.
The leading " and - appear to be for styling purposes. Keeping them in HTML or CSS could makes it easier to style or modify the view. This allows front-end developers to adjust the UI without changing any JavaScript code.
| ]; | ||
|
|
||
| // getting access from the html | ||
| const showquote = document.querySelector("displayArea"); |
There was a problem hiding this comment.
Where is this variable being used?
| //console.log(quoteGenerator); | ||
| //console.log(quot); | ||
| //console.log(Author); | ||
| quoteGenerator(); | ||
| //event handler |
There was a problem hiding this comment.
Best practice is to remove all unused code and unnecessary comments to keep the code clean.
cjyuan
added
Reviewed
Dagim-Daniel
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
Could you also check if all variable names adhere to the lower camel case convention? (I counted at least three names should be updated)
cjyuan
removed
the
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
quote_generator task under sprint 3 has been done