Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes a hover highlight issue on the attachment list component by adjusting the SCSS and expands the example page for this component. My review includes a suggestion to align the example component's state management with the project's style guide by using Angular signals.
| noPreviewAttachments: Attachment[] = [ | ||
| { | ||
| name: 'budget-quarterly.pptx' | ||
| }, | ||
| { | ||
| name: 'data.json' | ||
| }, | ||
| { | ||
| name: 'barchart.png' | ||
| }, | ||
| { | ||
| name: 'event.ics' | ||
| } | ||
| ]; |
There was a problem hiding this comment.
According to the repository's style guide (line 34), signals should be used for local component state. Consider converting noPreviewAttachments, and other state properties like attachments and readOnlyAttachments, to use signals. This would align with modern Angular practices and the project's conventions.
For example, you could define noPreviewAttachments as a signal:
import { signal } from '@angular/core';
// ...
noPreviewAttachments = signal<Attachment[]>([
{
name: 'budget-quarterly.pptx'
},
// ... other attachments
]);Then in your template, you would access it by calling the signal: [attachments]="noPreviewAttachments()".
Similarly, for the mutable attachments property, you could change it to a signal and update it like this:
// In SampleComponent
attachments = signal<Attachment[]>([]);
onRemoveAttachment(attachment: Attachment): void {
this.logEvent(`Remove attachment: ${attachment.name}`);
this.attachments.update(attachments => attachments.filter(a => a !== attachment));
}References
- The style guide recommends using signals for local component state (line 34). (link)
48bf030 to
968c862
Compare
968c862 to
ed8de15
Compare
Related to #1412
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: