London | 26-ITP-May | Rizqah Popoola | Sprint 2 |Book Library - #549
London | 26-ITP-May | Rizqah Popoola | Sprint 2 |Book Library#549risikatpopoola wants to merge 6 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
cjyuan
left a comment
There was a problem hiding this comment.
Solid start.
I think there are some improvements you could still make.
| @@ -1,46 +1,58 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Could we declare myLibrary in a way that prevents it from being accidentally reassigned?
There was a problem hiding this comment.
I have now declared this using const
| type="submit" | ||
| value="Submit" | ||
| class="btn btn-primary" | ||
| onclick="submit();" |
There was a problem hiding this comment.
Could you research the trade-off between
- attaching event listener in HTML
- attaching event listener usng
.addEventListener()in JS?
There was a problem hiding this comment.
It is easier to manage event listeners in js and it provides more control.
| titleInput.value.trim(), | ||
| authorInput.value.trim(), |
There was a problem hiding this comment.
A better and safer approach to deal with user input is to first preprocess/sanitise them, and then store the cleaned values in variables. Thereafter, refer only to the variables for cleaned values consistently throughout the rest of the code.
| const submitButton = document.getElementById("submit"); | ||
|
|
||
| submitButton.addEventListener("click", submit); |
There was a problem hiding this comment.
Why attach the event listener to the submit button here?
There was a problem hiding this comment.
I completely removed the event listener for the submit button and added it to the book form submission instead.
There was a problem hiding this comment.
How many times do you expect the app to add the event listeners to the submit button?
There was a problem hiding this comment.
It should only be once, i have now taken it out of setup
|
Please note that if you have made any changes, you need to push them to GitHub. |
| } | ||
| } | ||
|
|
||
| bookForm.addEventListener("submit", addBook); |
There was a problem hiding this comment.
Could consider placing all code that runs once on page load in a single function. For example, you could put it inside the page load callback or create a function named init() or setup() and call it once when the page loads.
This makes it easier to locate and manage all the code that runs once when the app starts.
Learners, PR Template
Self checklist
Changelist
In this PR, i have fixed all the bugs in script.js and refactored the script to be more efficient. I have also validated the html code to ensure it's free of errors.