UKHO / admiralty-design-system

MIT License
5 stars 0 forks source link

[BUG] Admiralty-Select: Issues with change detection when compared to standard select element #126

Open developernm opened 1 year ago

developernm commented 1 year ago

When trying to dynamically create a reactive form to render the admiralty-select component, we have an issue where it returns an error of NG0100 which can be seen below:

image

Here is the HTML specific to the design system component but using the disableOption method:

  <admiralty-select formControlName='methodOfSurveySelected'
                    id="methodOfSurvey"
                    [invalid]="getMethodOfSurveySelected(index).invalid && getMethodOfSurveySelected(index).touched"
                    [invalidMessage]="validate(getMethodOfSurveySelected(index), 'Method of survey')"
                    class="is-full-width mr-4"
                    (admiraltyChange)="onMethodOfSurveyChange($any($event.target).value, MethodOfSurveys, index)"
                    [label]="'Method of survey' + (methodsOfSurvey.length > 1 ? ' ' + (index + 1) : '') ">
    <option disabled></option>
    <ng-container *ngFor="let methodOfSurvey of MethodOfSurveys">
      <option [disabled]="disableOption(methodOfSurvey)" [ngValue]="methodOfSurvey.method">{{ methodOfSurvey.method }}</option>
    </ng-container>
  </admiralty-select>
  public disableOption(methodOfSurvey): boolean {
    return this.unavailableMethodsOfSurvey.includes(methodOfSurvey);
  }

Here is the HTML specific to the design system component by using the getAvailableMethodsOfSurvey(index) method to mutate the data using rxjs:

  <admiralty-select *ngIf="(getAvailableMethodsOfSurvey(index) | async) as MethodOfSurveys"
                    formControlName='methodOfSurveySelected'
                    id="methodOfSurvey"
                    [invalid]="getMethodOfSurveySelected(index).invalid && getMethodOfSurveySelected(index).touched"
                    [invalidMessage]="validate(getMethodOfSurveySelected(index), 'Method of survey')"
                    class="is-full-width mr-4"
                    (admiraltyChange)="onMethodOfSurveyChange($any($event.target).value, MethodOfSurveys, index)"
                    [label]="'Method of survey' + (methodsOfSurvey.length > 1 ? ' ' + (index + 1) : '') ">
    <option disabled></option>
    <ng-container *ngFor="let methodOfSurvey of MethodOfSurveys">
      <option [ngValue]="methodOfSurvey.method">{{ methodOfSurvey.method }}</option>
    </ng-container>
  </admiralty-select>
  public getAvailableMethodsOfSurvey(index: number): Observable<SurveyMethodDto[]> {
    return this.methodsOfSurveySubject$.pipe(
      takeWhile((methods: SurveyMethodDto[]): boolean => methods !== null),
      map((methods: SurveyMethodDto[]): SurveyMethodDto[] => {
        if (
          this.unavailableMethodsOfSurvey.length > 0 &&
          this.getMethodOfSurvey(index).value !== null &&
          this.getMethodOfSurvey(index).value !== undefined
        ) {
          const availableItems: SurveyMethodDto[] = methods.filter(
            (surveyMethod: SurveyMethodDto) => !this.unavailableMethodsOfSurvey.includes(surveyMethod)
          );
          const selectedItem: SurveyMethodDto = methods.filter(
            (surveyMethod: SurveyMethodDto): boolean =>
              this.getMethodOfSurvey(index).value.method === surveyMethod.method
          )[0];
          availableItems.push(selectedItem);
          return availableItems;
        } else {
          return methods.filter(
            (surveyMethod: SurveyMethodDto) => !this.unavailableMethodsOfSurvey.includes(surveyMethod)
          );
        }
      })
    );
  }

As you can see the 2 different methods cause the issue when returning back to the component regardless of whether or not you destroy the component. The issue lies between the "Current Value" and the "Old Value" changing after the content is rendered, when using the "admiralty-select". So in both methods we have both the current value and old value, whether it be (true changing false) or (an array of 10 changing to an array of 5).

The only way I managed to supress the error was implementing the following, however its not best practice to do this as it takes a big performance hit (literally being called at every change not matter how big or small - even when you take it out of the subscribe, which I implemented to limit the times it was called):

  constructor(private cdref: ChangeDetectorRef) {}

  ngAfterContentChecked(): void {
    this.isEditing$.subscribe((isEditing) => isEditing ?? this.cdref.detectChanges());
  }

When I removed this admiralty-select component and used a standard select component we have no issues or errors, with either of the 2 methods.

  <select formControlName='methodOfSurveySelected'
                    id="methodOfSurvey"
                    class="is-full-width mr-4"
                    (change)="onMethodOfSurveyChange($any($event.target).value, MethodOfSurveys, index)">
    <option disabled></option>
    <ng-container *ngFor="let methodOfSurvey of MethodOfSurveys">
      <option [disabled]="disableOption(methodOfSurvey)" [value]="methodOfSurvey.method">{{ methodOfSurvey.method }}</option>
    </ng-container>
  </select>

We need the admiralty-select component to be able to handle changes without modifying the changeDetection strategy or using the detectChanges method.

KatiePUX commented 6 months ago

Katie to liaise with Naran re: whether this is still an issue

KatiePUX commented 6 months ago

Liaised with Naran, low priority issues