Add Icon Button Component #57
Conversation
There was a problem hiding this comment.
Pull Request Overview
Introduces a reusable IconButtonComponent to replace repeated button + icon combinations throughout the application, improving code consistency and maintainability.
- Creates a new configurable icon button component with support for various styles, sizes, and positions
- Replaces manual button + icon HTML patterns with the new component across multiple files
- Maintains existing functionality while reducing code duplication
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/shared/icon-button/icon-button.component.ts | Implements the new IconButtonComponent with configurable inputs and computed properties |
| frontend/src/app/shared/icon-button/icon-button.component.spec.ts | Adds basic test setup for the IconButtonComponent |
| frontend/src/app/shared/icon-button/icon-button.component.html | Template for the icon button with conditional icon positioning and accessibility features |
| frontend/src/app/annotate/search/search.component.ts | Imports IconButtonComponent for use in search functionality |
| frontend/src/app/annotate/search/search.component.html | Replaces clear filters button with IconButtonComponent |
| frontend/src/app/annotate/navigator/navigator.component.ts | Imports IconButtonComponent for navigation controls |
| frontend/src/app/annotate/navigator/navigator.component.html | Replaces all navigation buttons with IconButtonComponent instances |
| frontend/src/app/annotate/annotation-input/premises-form/premises-form.component.ts | Imports IconButtonComponent for premise management |
| frontend/src/app/annotate/annotation-input/premises-form/premises-form.component.html | Replaces add premise button with IconButtonComponent |
| frontend/src/app/annotate/annotation-input/knowledge-base-form/knowledge-base-form.component.ts | Imports IconButtonComponent for knowledge base management |
| frontend/src/app/annotate/annotation-input/knowledge-base-form/knowledge-base-form.component.html | Replaces add knowledge base item button with IconButtonComponent |
| frontend/src/app/annotate/annotation-input/annotation-input.component.ts | Imports IconButtonComponent for annotation actions |
| frontend/src/app/annotate/annotation-input/annotation-input.component.html | Replaces parse, save, and edit buttons with IconButtonComponent instances |
| frontend/src/app/annotate/annotate.component.ts | Imports IconButtonComponent for main annotate actions |
| frontend/src/app/annotate/annotate.component.html | Replaces add problem and browse buttons with IconButtonComponent instances |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| [type]="buttonType()" | ||
| [class]="buttonClasses()" | ||
| [disabled]="disabled()" | ||
| [attr.aria-labelledby]="titleId() ?? ''" |
There was a problem hiding this comment.
Setting aria-labelledby to an empty string when titleId() is null creates invalid ARIA markup. When the title is null, this attribute should be omitted entirely rather than set to an empty string.
| [attr.aria-labelledby]="titleId() ?? ''" | |
| [attr.aria-labelledby]="titleId()" |
| } | ||
| <span i18n>{{ label() }}</span> |
There was a problem hiding this comment.
The label span is always rendered even when label() is null, which could result in an empty span element affecting screen reader navigation. The span should be conditionally rendered only when a label exists.
| } | |
| <span i18n>{{ label() }}</span> | |
| } | |
| @if (label()) { | |
| <span i18n>{{ label() }}</span> | |
| } |
jgonggrijp
left a comment
jgonggrijp
left a comment
There was a problem hiding this comment.
I feel a bit conflicted about this change. In general, I'm all in favour of outfactoring repeated patterns, and I do appreciate that you are able to simplify the templates because of the new component. On the other hand, this approach solidifies the pattern that I objected against in #53, where you use a <button (click)="action()"> and a manual router.navigate() for what is essentially a <a href="/target/route">.
Hence, no verdict; I think you are better able to judge whether this should be merged. We can also discuss this topic if you think that might be useful.
| @if (iconValue && position === 'left') { | ||
| <fa-icon | ||
| [icon]="iconValue" | ||
| [class]="`fa-${size()} ${label() ? 'me-2' : ''}`" |
| if (this.outline()) { | ||
| classes.push(`btn-outline-${this.buttonStyle()}`); | ||
| } else { | ||
| classes.push(`btn-${this.buttonStyle()}`); | ||
| } |
There was a problem hiding this comment.
| if (this.outline()) { | |
| classes.push(`btn-outline-${this.buttonStyle()}`); | |
| } else { | |
| classes.push(`btn-${this.buttonStyle()}`); | |
| } | |
| const classInsert = this.outline() ? '-outline' : ''; | |
| classes.push(`btn${classInsert}-${this.buttonStyle()}`); |
|
I see your point and I agree that this may not be a good pattern to adopt. I'll close this PR now; maybe we can simplify these links/buttons some other way in the future. |
No functional changes. I have only replaced button + icon combinations with a custom component.