Skip to content

fix(attachment-list): hover highlight#1670

Open
robertwilde wants to merge 1 commit intomainfrom
fix/attachment-list/hover-state
Open

fix(attachment-list): hover highlight#1670
robertwilde wants to merge 1 commit intomainfrom
fix/attachment-list/hover-state

Conversation

@robertwilde
Copy link
Collaborator

@robertwilde robertwilde commented Mar 17, 2026

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +57 to +70
noPreviewAttachments: Attachment[] = [
{
name: 'budget-quarterly.pptx'
},
{
name: 'data.json'
},
{
name: 'barchart.png'
},
{
name: 'event.ics'
}
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. The style guide recommends using signals for local component state (line 34). (link)

@robertwilde robertwilde force-pushed the fix/attachment-list/hover-state branch 3 times, most recently from 48bf030 to 968c862 Compare March 19, 2026 14:54
@robertwilde robertwilde force-pushed the fix/attachment-list/hover-state branch from 968c862 to ed8de15 Compare March 20, 2026 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants