Conversation
Daniel-S-Developer
left a comment
Daniel-S-Developer
left a comment
There was a problem hiding this comment.
Hello everyone,
This was a fun little exercise! I hope my review meets your standards. Looking forward to your feedback and our upcoming chat.
Cheers,
Daniel
| @@ -0,0 +1,56 @@ | |||
| import { Injectable, signal } from '@angular/core'; | |||
There was a problem hiding this comment.
The current implementation introduces a mix of patterns by using a custom service for form logic while the template relies on [ngModel]. For better maintainability and adherence to Angular standards, we should commit to one of two idiomatic solutions: either fully adopt Reactive Forms by replacing the service with a FormGroup, or simplify to a pure Template-Driven approach since the template already uses ngModel.
Furthermore, the architectural choice to provide the CommentFormService at the 'root' level is a significant issue. This creates a single, global instance for a state that should be local to each comment form, which would lead to bugs like shared input between different components. Even if we were to keep the service instead of refactoring to a standard forms approach, its scope would need to be corrected: it should be provided directly at the component level to ensure each instance gets its own private state.
| }) | ||
| export class TaskCommentsComponent implements OnInit, OnDestroy { | ||
| private readonly taskService = inject(TaskService); | ||
| readonly commentFormService = inject(CommentFormService); |
There was a problem hiding this comment.
See comment in CommentFormService
| /** | ||
| * Component for displaying and managing task comments | ||
| */ | ||
| @Component({ |
There was a problem hiding this comment.
This component is acting as a "Smart Component" by injecting services and fetching its own data. For better reusability and separation of concerns, this should be a "Dumb Component." It should receive data via input signal and emit events via output signal. The parent component should be responsible for handling the business logic.
| private refreshSubscription?: Subscription; | ||
|
|
||
|
|
||
| private readonly commentsSignal = signal<Comment[]>([]); |
There was a problem hiding this comment.
This signal creates a local copy of the comments, separate from the TaskService. This leads to two sources of truth and requires manual synchronization (loadComments). The component should instead directly use or derive its data from the service to ensure data consistency. This will be resolved by refactoring to a Dumb Component.
| ngOnInit(): void { | ||
| this.loadComments(); | ||
|
|
||
| this.refreshSubscription = interval(30000).subscribe(() => { |
There was a problem hiding this comment.
This interval is never unsubscribed. The ngOnDestroy method is empty, which means this subscription will live on forever, even after the component is destroyed. This will lead to performance degradation and unpredictable behavior.
| ]; | ||
|
|
||
| // Add sample comments to first task | ||
| sampleTasks[0].comments = [ |
There was a problem hiding this comment.
This large block of sample data makes the service harder to read. To improve separation of concerns, this mock data could be extracted into a separate file (e.g., app/data/mock-tasks.ts) and imported here. This would keep the service focused on logic and make the data easier to manage.
| @@ -0,0 +1,111 @@ | |||
| .comments-container { | |||
There was a problem hiding this comment.
This file uses hardcoded values for colors (e.g., #666, #999) and spacing (e.g., 16px, 24px). For better maintainability and consistency across the application, consider extracting these "magic numbers" into SCSS variables (design tokens) in a shared file (e.g., _variables.scss). This would make it easier to manage the theme and ensure a consistent design system.
|
|
||
| <mat-divider></mat-divider> | ||
|
|
||
| <div class="comment-form" *ngIf="true"> |
There was a problem hiding this comment.
This *ngIf="true" is redundant and can be removed completely.
If conditional logic is needed in the future, it should be implemented using the new @if block syntax for consistency with the rest of the template.
| <div class="comment-form" *ngIf="true"> | ||
| <mat-form-field appearance="outline" class="author-field"> | ||
| <mat-label>Your Name</mat-label> | ||
| <input |
There was a problem hiding this comment.
This form is manually bound to the CommentFormService using [ngModel] and (ngModelChange). This approach is discouraged as it mixes template-driven patterns with a custom service that reinvents Angular's form handling capabilities.
Recommendation:
This entire form should be refactored to use either Reactive Forms (with [formGroup]) or a purely Template-Driven approach. This would eliminate the need for the CommentFormService, as discussed in the review comments for that service file.
| <!-- Comments List --> | ||
| <div class="comments-list"> | ||
| @if (hasComments()) { | ||
| @for (comment of sortedComments(); track trackByCommentId($index, comment)) { |
There was a problem hiding this comment.
The trackBy function call is not necessary here. The new @for syntax allows for a more direct and cleaner approach.
Recommendation:
Change this to: @for (comment of sortedComments(); track comment.id) This allows you to remove the trackByCommentId method from the component class.
2 participants
As discussed, I added the comments feature to the tasks