Conversation
There was a problem hiding this comment.
Pull request overview
Updates ngx-loading-buttons for a newer Angular version and refactors directives/playground to use signal-based APIs.
Changes:
- Bumps Angular/tooling dependencies (and TypeScript) in
package.json. - Refactors
MatBasicSpinnerDirectiveandMatGlowDirectiveto useinput()+effect()and host class bindings. - Updates the playground component/template to use a
signal<boolean>forsaving.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/ngx-loading-buttons/src/public-api.ts | Switches public exports to named exports. |
| projects/ngx-loading-buttons/src/lib/mat-glow/mat-glow.directive.ts | Migrates to signal inputs/effects and host bindings for classes/disabled state. |
| projects/ngx-loading-buttons/src/lib/mat-basic-spinner.directive.ts | Migrates to signal inputs/effects and host bindings for classes/disabled state. |
| projects/ngx-loading-buttons-playground/src/app/app.component.ts | Converts saving to a signal and updates click handler accordingly. |
| projects/ngx-loading-buttons-playground/src/app/app.component.html | Updates bindings to call saving() in templates. |
| package.json | Updates Angular/dev tooling versions for the upgrade. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Directive({ | ||
| selector: '[mtBasicSpinner]', | ||
| standalone: true, | ||
| host: { | ||
| '[class.mat-spinner]': 'mtBasicSpinner()', | ||
| '[class.hide-btn-text]': 'hideText() && mtBasicSpinner()', | ||
| }, | ||
| }) |
There was a problem hiding this comment.
NgxLoadingButtonsModule currently imports MatBasicSpinnerDirective as a standalone directive (see projects/ngx-loading-buttons/src/lib/ngx-loading-buttons.module.ts:6-13). This directive no longer sets standalone: true, so the module (and any consumer importing the directive directly) will fail to compile. Either restore standalone: true here, or update the module to declare/export the directive via declarations instead of imports.
| @Directive({ | ||
| selector: '[mtGlow]', | ||
| standalone: true, | ||
| host: { | ||
| '[class.mat-glow]': 'mtGlow()', | ||
| '[class.hide-btn-text]': 'hideText() && mtGlow()', | ||
| } | ||
| }) |
There was a problem hiding this comment.
NgxLoadingButtonsModule currently imports MatGlowDirective as a standalone directive (see projects/ngx-loading-buttons/src/lib/ngx-loading-buttons.module.ts:6-13). This directive no longer sets standalone: true, so the module (and any consumer importing the directive directly) will fail to compile. Either restore standalone: true here, or update the module to declare/export the directive via declarations instead of imports.
| constructor(elem: ElementRef<HTMLButtonElement>) { | ||
| effect(() => { | ||
| elem.nativeElement.disabled = this.mtBasicSpinner(); | ||
| }); |
There was a problem hiding this comment.
This directive now uses effect() in the constructor. effect() must run inside an Angular injection context, so tests (and any code) that instantiate the directive via new MatBasicSpinnerDirective(...) will throw at runtime. Update tests/usage to create the directive via TestBed/template instantiation (or wrap construction in runInInjectionContext).
| title = 'ngx-loading-buttons-playground'; | ||
| saving = false; | ||
| saving = signal(false); | ||
| color = "blue"; | ||
|
|
||
| click(): void { | ||
| this.saving = true; | ||
| setTimeout(() => this.saving = false, 3000); | ||
| this.saving.set(true); | ||
| setTimeout(() => this.saving.set(false), 3000); |
There was a problem hiding this comment.
saving was changed from a boolean to a signal<boolean>, but the existing directive unit test (projects/ngx-loading-buttons/src/lib/mat-basic-spinner.directive.spec.ts) assigns fixture.componentInstance.saving = true/false and relies on it being a boolean. Update the test (and any other consumers) to use saving.set(...) (or switch the test component back to a boolean) to avoid failing CI.
| constructor(elem: ElementRef<HTMLButtonElement>) { | ||
| effect(() => { | ||
| document.documentElement.style.setProperty('--glow-color', this.glowColor()); | ||
| }); | ||
| effect(() => { | ||
| elem.nativeElement.disabled = this.mtGlow(); | ||
| }); |
There was a problem hiding this comment.
This directive now uses effect() in the constructor. effect() must run inside an Angular injection context, so any tests/usage that instantiate the directive via new MatGlowDirective(...) will throw at runtime. Ensure the directive is created via template/TestBed (or wrap construction in runInInjectionContext).
No description provided.