Repository navigation
Conversation
rafasilpereira
left a comment
rafasilpereira
left a comment
There was a problem hiding this comment.
Great to see you went beyond the assignment by also scaffolding task-list-ts! For future PRs, it's better to keep each task in its own PR — having both apps in one diff makes it harder to review. The actual assignment (task-1) would be much easier to evaluate on its own.
| id: string; | ||
| title: string; | ||
| description: string; | ||
| completed: boolean; |
There was a problem hiding this comment.
I would add also here createdAt
| onDelete(id: string): void; | ||
| onEdit(id: string, title: string, description: string): void; | ||
| onFilter(status: FilterStatus): void; | ||
| } |
There was a problem hiding this comment.
Good approach on creating types.ts file!
| function render() { | ||
| appView.render(taskManager.getFiltered(currentFilter), currentFilter); | ||
| } |
| render(tasks: Task[], activeFilter: FilterStatus) { | ||
| this.taskListView.render(tasks); | ||
| this.updateFilterButtons(activeFilter); | ||
| } | ||
|
|
||
| updateFilterButtons(activeFilter) { | ||
| const buttons = this.filterBar.querySelectorAll("[data-filter]"); | ||
| updateFilterButtons(activeFilter: FilterStatus) { | ||
| const buttons = | ||
| this.filterBar.querySelectorAll<HTMLButtonElement>("[data-filter]"); | ||
| for (const btn of buttons) { | ||
| btn.classList.toggle("active", btn.dataset.filter === activeFilter); | ||
| } |
There was a problem hiding this comment.
Both methods doesn't have a return type
| export function enterEditMode( | ||
| li: HTMLLIElement, | ||
| task: Task, | ||
| handlers: TaskHandlers, | ||
| ) { | ||
| li.innerHTML = String.raw` | ||
| <input type="checkbox"${task.completed ? " checked" : ""}> | ||
| <div class="edit-form"> | ||
| <input type="text" class="edit-title" value="${escapeHtml(task.title)}"> | ||
| <input type="text" class="edit-desc" value="${escapeHtml(task.description)}"> | ||
| <div class="edit-actions"> | ||
| <button class="save-btn">Save</button> | ||
| <button class="cancel-btn">Cancel</button> | ||
| </div> | ||
| </div> | ||
| `; | ||
|
|
||
| const checkbox = li.querySelector<HTMLInputElement>( | ||
| "input[type='checkbox']", | ||
| )!; | ||
| const titleInput = li.querySelector<HTMLInputElement>(".edit-title")!; | ||
| const descInput = li.querySelector<HTMLInputElement>(".edit-desc")!; | ||
|
|
||
| checkbox.addEventListener("change", () => handlers.onToggle(task.id)); | ||
|
|
||
| li.querySelector<HTMLButtonElement>(".save-btn")!.addEventListener( | ||
| "click", | ||
| () => { | ||
| const newTitle = titleInput.value.trim(); | ||
| if (!newTitle) return; | ||
| handlers.onEdit(task.id, newTitle, descInput.value.trim()); | ||
| }, | ||
| ); | ||
|
|
||
| li.querySelector<HTMLButtonElement>(".cancel-btn")!.addEventListener( | ||
| "click", | ||
| () => { | ||
| const activeFilter = | ||
| (document.querySelector<HTMLButtonElement>(".filter-bar .active") | ||
| ?.dataset.filter as FilterStatus) ?? "all"; | ||
| handlers.onFilter(activeFilter); | ||
| }, | ||
| ); | ||
|
|
||
| titleInput.focus(); | ||
| } |
There was a problem hiding this comment.
This is a common pattern, even the function is no return, add void
| onAdd(title, description) { | ||
| taskManager.addTask(title, description); | ||
| render(); |
There was a problem hiding this comment.
All parameters doesn't have a type in the methods
| for (const li of this.root.querySelectorAll<HTMLLIElement>(".task-item")) { | ||
| const id = (li as HTMLLIElement).dataset.id!; |
There was a problem hiding this comment.
The cast in line 38 is redudant since you are already specifying that you want a list element in line 37
| filterBar: HTMLDivElement; | ||
| taskListView: TaskListView; |
There was a problem hiding this comment.
I recommend the reading for encapsulation in TypeScript and how to protect the objects
rafasilpereira
left a comment
rafasilpereira
left a comment
There was a problem hiding this comment.
The build generates asset paths as /assets/index.js (absolute), which breaks when dist/index.html is opened and that doesn't root at the project folder. Adding a vite.config.ts with base: './' fixes this so the built app works anywhere:
import { defineConfig } from 'vite'
export default defineConfig({
base: './',
})
rafasilpereira
added
Reviewed
No description provided.