Open PhamNguyenDuyTien opened 1 year ago
Hello @PhamNguyenDuyTien , well done!. Below is some feedback for your assignment.
Final result: ✅ passed 90% of requirements
Using inline events like onclick
, oninput
, onchange
... in HTML is a bad practice. For your HTML, it's no problem because each event is used for a single element. But when you have to reuse them for multiple elements, your code will be hard to read and maintain. Consider addEventListener
instead. Research Why is using inline events in HTML a bad practice
for further information.
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L47C29-L47C29
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L74
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L40
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L79
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L72
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L115
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L124
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L128
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L136
Should use <button />
for clickable elements instead of <div />
, <span />
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L134-L139
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/index.html#L79-L81
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/script.js#L35C57-L35C57
Recommend to use rem
, em
for font-size instead of px
. It's better when you want to scale container and also meet web accessibility standards.
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/style.css#L38-L44
It's better to wrap JSON.parse
inside a try-catch
to prevent unexpected error from parsing data
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/script.js#L6
Avoid using innerHTML
to manipulate element, it's a bad practice. Consider alternative solutions like textContent
, appendChild
, setHTML
, sanitizer
... instead. Research keyword Why is using innerHTML in HTML is bad idea
and cross-site scripting vulnerabilities
.
https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/script.js#L32-L41
Are you sure that you won't get an unlucky for this random id? LOL. Why don't go with timestamp or uuid? https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/533ab53eeb97df9552688210ba205e101897e0f3/assignment-1/script.js#L120
While searching books, if a book is added or deleted, searching state will be reset but search value is still there. Resume searching state after book list is changed.
Link demo: https://df-frontend-2023-assignment-1-1.vercel.app