MinkNhat1 / df-frontend-2023

0 stars 0 forks source link

Submission for assignment 1 #1

Open MinkNhat1 opened 1 year ago

MinkNhat1 commented 1 year ago
zlatanpham commented 1 year ago

Hello @MinkNhat1, good start!

Requirements

Final result: ✅ passed 70% of the requirements

Feedback

We also have some comments for your work:

  1. Close button should have aria-label for better accessibility https://github.com/MinkNhat1/df-frontend-2023/blob/main/assignment-1/index.html#L65
  2. Should use <button/> for clickable elements image
  3. Noticed that you named an id as search-func. It's typically best to avoid naming based on functionality (i.e., func). Instead, try to describe the content or purpose. Perhaps search-input would be clearer.
  4. Try to avoid using inline JS method in the HTML. Move it to script.js and use addEventListener instead for the sake of separation of concern https://github.com/MinkNhat1/df-frontend-2023/blob/main/assignment-1/index.html#L22 image
    1. Ensure consistent usage of class names. You have txt-box, btn-yes, and del-func, which seem to mix different naming styles
    2. Instead of using inline styles to manage the visibility of the modals, consider using classes like .hidden and .visible. This way, the styles remain in the stylesheet, and your JS just toggles the classes. https://github.com/MinkNhat1/df-frontend-2023/blob/main/assignment-1/script.js#L21-L35
    3. This can refer to (5). The class btn-yes might be better named something like btn-primary to describe its purpose rather than its content.
    4. Avoid float. With modern layout techniques like Flexbox and Grid, the need for float is greatly reduced since floating elements can sometimes lead to layout issues. https://github.com/MinkNhat1/df-frontend-2023/blob/main/assignment-1/style.css#L146

🌟 Great job using event delegation for the delete buttons and the table body! This is an efficient way to handle events on dynamically added elements.