London | 26-ITP-May | Gideon Defar | Sprint 2 | Book Library - #558
London | 26-ITP-May | Gideon Defar | Sprint 2 | Book Library#558gideondefar wants to merge 5 commits into
Conversation
gideondefar
changed the title
gideondefar
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
According to https://validator.w3.org/, there are errors in your index.html. Could you fix these errors?
Other changes look good. You did your homework!
| bookForm.addEventListener("submit", addBook); | ||
|
|
||
| window.addEventListener("load", function (e) { | ||
| populateStorage(); | ||
| render(); | ||
| }); |
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.
| myLibrary.splice(i, 1); | ||
| render(); | ||
|
|
||
| alert(`You've deleted title: ${deletedTitle}`); |
There was a problem hiding this comment.
alert() is a blocking function call. As a result, invoking it prevents the browser from updating the UI until the dialog is dismissed.
If time permits, research for approaches that allows the UI to update before displaying the alert dialog. (This is an optional change).
There was a problem hiding this comment.
Allow the browser to update the UI before showing the blocking alert
cjyuan
added
Reviewed
gideondefar
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. Well done.
cjyuan
added
Complete
2 participants
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.