ZA Cape Town | ITP-May-2025 | Dawud Vermeulen | Sprint 2 | Book-Library #308
Conversation
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
Dave-Vermeulen
added
the
Needs Review
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
Doing so can help me speed up the review process. Thanks.
|
cjyuan
added
Reviewed
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
Dave-Vermeulen
changed the title
Dave-Vermeulen
changed the title
|
@cjyuan no mention in the |
Dave-Vermeulen
added
the
Needs Review
LonMcGregor
added
the
Review in progress
debugging/book-library/index.html
Outdated
There was a problem hiding this comment.
There are some things missing from your HTML file that should be added. Hint: how people are going to interact with your webpage in a web browser?
There is also some invalid HTML. Can you spot these? Hint: have you used an HTML validator before? https://validator.w3.org/
There was a problem hiding this comment.
@LonMcGregor this was actually a lot. Really useful site. So have update the file, please review and let me know if more is required.
debugging/book-library/script.js
Outdated
| title.value == "" || | ||
| pages.value == null || | ||
| pages.value == "" | ||
| titleInput.value.trim == "" || // cant be null so removed all that |
There was a problem hiding this comment.
There may be ways to improve your form validation here. Things to think about:
- Is your validation checking all the inputs?
- Is there a way to get the browser to do some of the checking for you, without needing to manually check for empty inputs?
There was a problem hiding this comment.
@LonMcGregor ah very DRY indeed. cool HTML5 tricks. have updated both files to handle the validation and submission a bit differently.
debugging/book-library/script.js
Outdated
| render(); | ||
| alert(`You've deleted title: ${deletedTitle}`); |
There was a problem hiding this comment.
Think about how deleting a file works on your desktop. Is being notified after the deletion normally how this works? Is there a better interaction you could use than an alert here?
There was a problem hiding this comment.
@LonMcGregor okay so this is bit higher grade. i think what you mean is that the users flow is interrupted when the alert pops up (clarify if im wrong kanala). based on this assumption and the fact that the delete works i have built a new function to display the notification and done away with the disruptive alert. The new one disappears after 3 secs.
There was a problem hiding this comment.
The alert is quite interrupting, yes, making it less "in your face" is one solution. The other is, if you really want something interrupting, to use something like confirm instead.
It sounds like you've made good progress here. I can't see the files yet on github though - have you comitted them all?
LonMcGregor
removed
Needs Review
|
Good work updating the HTML file. Can you also have a look at the form validation comment I made - that would be the minimum needed to fix to complete this sprint task. At minimum, could you ensure that invalid page values are checked properly? |
|
OK. Good to see the final version. I am happy with what you have finished with here. Well done, you are complete with this sprint |
LonMcGregor
added
Complete
Learners, PR Template
Self checklist
Changelist
Fixed the errors in the code. some were small some were bigger like the submit() function had problems with author being set to title so changed to author value. Then went on to the render() function and fixed the delButton use. A 3rd bug was on the delete buttons event listener there was a typo. Then finally the if statement logic in the render function was reversed so fixed that.
Questions
Please review my PR 🙏🏽