Skip to content

Commit

Permalink
chore(dotcms-ui): Fix error with autocomplete #30249 (#30269)
Browse files Browse the repository at this point in the history 
### Parent Issue
#30249 

### Proposed Changes
* Change singal approach

### The problem

This computed signal caused the error:

```ts
$filteredSuggestions = computed(() => {
    const classes = this.$classes();
    const query = this.$query();

    if (!query) {
        return classes;
    }

    return classes.filter((item) => item.includes(query));
});

filterClasses({ query }: AutoCompleteCompleteEvent): void {
    this.$query.set(query);
}
```

The signals functionality in PrimeNg doesn't works very well. In the
previous signal, `$filteredSuggestions` doesn't trigger a change if the
classes or query are the same as the current state. This is desirable.
However, PrimeNg
[waits](https://github.com/primefaces/primeng/blob/d6c7ffcfc5cb9a8ed355399ddae276c279836ccf/src/app/components/autocomplete/autocomplete.ts#L541)
for a change in the `suggestions` input after the `completeMethod` is
called to turn off the loading. This is why the loading indicator
remains active but never deactivates.

To address this, we manually forced a change in `$filteredSuggestions`
and used `[...filteredClasses]` to create a new array.

```ts
filterClasses({ query }: AutoCompleteCompleteEvent): void {
      const classes = this.$classes();
      const filteredClasses = query ? classes.filter((item) => item.includes(query)) : classes;
      this.$filteredSuggestions.set([...filteredClasses]);
  }
```

### Checklist
- [x] Tests
- [x] Translations
- [x] Security Implications Contemplated (add notes if applicable)

### Screenshots



https://github.com/user-attachments/assets/5a199569-9192-4acb-89bd-3fb6530aefce
  • Loading branch information
@nicobytes
nicobytes authored Oct 7, 2024
1 parent 0028f3c commit 3f8e75b
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 206 deletions.
12 changes: 6 additions & 6 deletions ...plate-builder/components/add-style-classes-dialog/add-style-classes-dialog.component.html
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
<div class="dialog__auto-complete-container field">
@let isJsonClasses = $isJsonClasses();
@let hasClasses = $hasClasses();
<p-autoComplete
(completeMethod)="filterClasses($event)"
[(ngModel)]="$selectedClasses"
[unique]="true"
[suggestions]="$filteredSuggestions()"
[size]="446"
[multiple]="true"
[dropdown]="isJsonClasses"
[dropdown]="hasClasses"
[autofocus]="true"
dropdownMode="blank"
class="p-fluid"
dotSelectItem
data-testId="autocomplete"
inputId="auto-complete-input"
appendTo="body" />
@if (isJsonClasses) {
appendTo="body"
dotSelectItem />
@if (hasClasses) {
<ul data-testId="list">
<li>
<i class="pi pi-info-circle"></i>
Expand Down
6 changes: 5 additions & 1 deletion ...plate-builder/components/add-style-classes-dialog/add-style-classes-dialog.component.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,13 @@
justify-content: flex-end;
}

p-autoComplete {
:host ::ng-deep p-autocomplete {
margin-bottom: $spacing-3;
display: block;
.p-autocomplete-dropdown.p-element.p-button {
border-top-left-radius: 0;
border-bottom-left-radius: 0;
}
}

ul {
Expand Down
98 changes: 3 additions & 95 deletions ...te-builder/components/add-style-classes-dialog/add-style-classes-dialog.component.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expect, it } from '@jest/globals';
import { createFakeEvent } from '@ngneat/spectator';
import { Spectator, byTestId, createComponentFactory, mockProvider } from '@ngneat/spectator/jest';
import { of, throwError } from 'rxjs';
import { of } from 'rxjs';

import { provideHttpClient } from '@angular/common/http';
import { FormsModule } from '@angular/forms';
Expand Down Expand Up @@ -68,7 +68,7 @@ describe('AddStyleClassesDialogComponent', () => {
}
}),
mockProvider(JsonClassesService, {
getClasses: jest.fn().mockReturnValue(of({ classes: ['class1', 'class2'] }))
getClasses: jest.fn().mockReturnValue(of(['class1', 'class2']))
})
]
});
Expand All @@ -84,7 +84,6 @@ describe('AddStyleClassesDialogComponent', () => {
expect(autocomplete.unique).toBe(true);
expect(autocomplete.autofocus).toBe(true);
expect(autocomplete.multiple).toBe(true);
expect(autocomplete.size).toBe(446);
expect(autocomplete.inputId).toBe('auto-complete-input');
expect(autocomplete.appendTo).toBe('body');
expect(autocomplete.dropdown).toBe(true);
Expand Down Expand Up @@ -170,7 +169,7 @@ describe('AddStyleClassesDialogComponent', () => {
provide: JsonClassesService,
useValue: {
getClasses() {
return of({ classes: [] });
return of([]);
}
}
}
Expand Down Expand Up @@ -200,95 +199,4 @@ describe('AddStyleClassesDialogComponent', () => {
expect(list.textContent).toContain('no suggestions setup suggestions');
});
});

describe('bad format json', () => {
beforeEach(() => {
spectator = createComponent({
providers: [
...providers,
{
provide: DynamicDialogConfig,
useValue: {
data: {
selectedClasses: []
}
}
},
{
provide: JsonClassesService,
useValue: {
getClasses() {
return of({ badFormat: ['class1'] });
}
}
}
]
});

jsonClassesService = spectator.inject(JsonClassesService, true);
dialogRef = spectator.inject(DynamicDialogRef);
autocomplete = spectator.query(AutoComplete);
});

it('should set dropdown to false in autocomplete', () => {
spectator.detectChanges();
expect(autocomplete.dropdown).toBe(false);
});

it('should set component.classes empty', () => {
spectator.detectChanges();

expect(spectator.component.$classes()).toEqual([]);
});

it('should have multiples help message', () => {
spectator.detectChanges();
const list = spectator.query(byTestId('list'));

expect(list.textContent).toContain('no suggestions setup suggestions');
});
});

describe('error', () => {
beforeEach(() => {
spectator = createComponent({
providers: [
...providers,
{
provide: DynamicDialogConfig,
useValue: {
data: {
selectedClasses: []
}
}
},
{
provide: JsonClassesService,
useValue: {
getClasses() {
return throwError(
new Error('An error occurred while fetching classes')
);
}
}
}
]
});

jsonClassesService = spectator.inject(JsonClassesService, true);
dialogRef = spectator.inject(DynamicDialogRef);
autocomplete = spectator.query(AutoComplete);
});

it('should set dropdown to false in autocomplete', () => {
spectator.detectChanges();
expect(autocomplete.dropdown).toBe(false);
});

it('should set component.classes empty', () => {
spectator.detectChanges();

expect(spectator.component.$classes()).toEqual([]);
});
});
});
15 changes: 7 additions & 8 deletions ...builder/components/add-style-classes-dialog/add-style-classes-dialog.component.stories.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Meta, moduleMetadata, StoryObj, applicationConfig } from '@storybook/angular';
import { of } from 'rxjs';

import { HttpClient, provideHttpClient } from '@angular/common/http';
import { provideHttpClient } from '@angular/common/http';
import { FormsModule } from '@angular/forms';
import { BrowserAnimationsModule } from '@angular/platform-browser/animations';

Expand Down Expand Up @@ -46,18 +46,17 @@ const meta: Meta<AddStyleClassesDialogComponent> = {
}
}
},
{
provide: HttpClient,
useValue: {
get: (_: string) => of(MOCK_STYLE_CLASSES_FILE)
}
},
{
provide: DotMessageService,
useValue: DOT_MESSAGE_SERVICE_TB_MOCK
},
DynamicDialogRef,
JsonClassesService
{
provide: JsonClassesService,
useValue: {
getClasses: () => of(MOCK_STYLE_CLASSES_FILE.classes)
}
}
]
})
]
Expand Down
81 changes: 14 additions & 67 deletions ...emplate-builder/components/add-style-classes-dialog/add-style-classes-dialog.component.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { of } from 'rxjs';

import {
ChangeDetectionStrategy,
Component,
Expand All @@ -11,12 +9,10 @@ import {
import { toSignal } from '@angular/core/rxjs-interop';
import { FormsModule } from '@angular/forms';

import { AutoCompleteModule } from 'primeng/autocomplete';
import { AutoCompleteCompleteEvent, AutoCompleteModule } from 'primeng/autocomplete';
import { ButtonModule } from 'primeng/button';
import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog';

import { catchError, map, shareReplay, tap } from 'rxjs/operators';

import { DotMessagePipe, DotSelectItemDirective } from '@dotcms/ui';

import { JsonClassesService } from './services/json-classes.service';
Expand Down Expand Up @@ -49,84 +45,36 @@ export class AddStyleClassesDialogComponent implements OnInit {
* @memberof AddStyleClassesDialogComponent
*/
readonly #dialogRef = inject(DynamicDialogRef);
readonly #dynamicDialogConfig = inject(DynamicDialogConfig<{ selectedClasses: string[] }>);
/**
* Selected classes to be added
* @memberof AddStyleClassesDialogComponent
*/
$selectedClasses = signal<string[]>([]);
/**
* Classes to be displayed
*
* @memberof AddStyleClassesDialogComponent
*/
$classes = signal<string[]>([]);
/**
* Query to filter the classes
*
* @memberof AddStyleClassesDialogComponent
*/
$query = signal<string | null>(null);
/**
* Filtered suggestions based on the query
* Check if the JSON file has classes
*
* @memberof AddStyleClassesDialogComponent
*/
$filteredSuggestions = computed(() => {
const classes = this.$classes();
const query = this.$query();

if (!query) {
return classes;
}

return classes.filter((item) => item.includes(query));
$classes = toSignal(this.#jsonClassesService.getClasses(), {
initialValue: []
});
/**
* Check if there are classes in the JSON file
*
* @memberof AddStyleClassesDialogComponent
*/
isJsonClasses$ = this.#jsonClassesService.getClasses().pipe(
tap(({ classes }) => {
if (classes?.length) {
this.$classes.set(classes);
} else {
this.$classes.set([]);
}
}),
map(({ classes }) => !!classes?.length),
catchError(() => {
this.$classes.set([]);

return of(false);
}),
shareReplay(1)
);
/**
* Check if the JSON file has classes
* Filtered suggestions based on the query
*
* @memberof AddStyleClassesDialogComponent
*/
$isJsonClasses = toSignal(this.isJsonClasses$, {
initialValue: false
});
$filteredSuggestions = signal<string[]>(this.$classes());

constructor(
public dynamicDialogConfig: DynamicDialogConfig<{
selectedClasses: string[];
}>
) {}
$hasClasses = computed(() => this.$classes().length > 0);

/**
* Set the selected classes
*
* @memberof AddStyleClassesDialogComponent
*/
ngOnInit() {
const data = this.dynamicDialogConfig.data;
if (data) {
this.$selectedClasses.set(data.selectedClasses);
}
this.$selectedClasses.set(this.#dynamicDialogConfig?.data?.selectedClasses || []);
}

/**
Expand All @@ -136,15 +84,14 @@ export class AddStyleClassesDialogComponent implements OnInit {
* @return {*}
* @memberof AddStyleClassesDialogComponent
*/
filterClasses({ query }: { query: string }): void {
filterClasses({ query }: AutoCompleteCompleteEvent): void {
/*
https://github.com/primefaces/primeng/blob/master/src/app/components/autocomplete/autocomplete.ts#L739
https://github.com/primefaces/primeng/blob/master/src/app/components/autocomplete/autocomplete.ts#L541
Sadly we need to pass suggestions all the time, even if they are empty because on the set is where the primeng remove the loading icon
*/

// PrimeNG autocomplete doesn't support async pipe in the suggestions
this.$query.set(query);
const classes = this.$classes();
const filteredClasses = query ? classes.filter((item) => item.includes(query)) : classes;
this.$filteredSuggestions.set([...filteredClasses]);
}

/**
Expand Down
Loading

0 comments on commit 3f8e75b

Please sign in to comment.