Skip to content

Commit

Permalink
fix: Redundant reacreation of cart items form (#18962)
Browse files Browse the repository at this point in the history
  • Loading branch information
sdrozdsap authored Jul 2, 2024
1 parent 83ebd99 commit 94926b6
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,13 @@ import {
PromotionLocation,
SelectiveCartFacade,
} from '@spartacus/cart/base/root';
import { I18nTestingModule, UserIdService } from '@spartacus/core';
import {
FeatureConfigService,
I18nTestingModule,
UserIdService,
} from '@spartacus/core';
import { OutletContextData, PromotionsModule } from '@spartacus/storefront';
import { Observable, of } from 'rxjs';
import { Observable, Subject, of } from 'rxjs';
import { CartItemListComponent } from './cart-item-list.component';

class MockActiveCartService {
Expand Down Expand Up @@ -107,6 +111,12 @@ const mockContext = {
};
const context$ = of(mockContext);

class MockFeatureConfigService {
isEnabled() {
return true;
}
}

describe('CartItemListComponent', () => {
let component: CartItemListComponent;
let fixture: ComponentFixture<CartItemListComponent>;
Expand All @@ -132,6 +142,7 @@ describe('CartItemListComponent', () => {
{ provide: SelectiveCartFacade, useValue: mockSelectiveCartService },
{ provide: MultiCartFacade, useClass: MockMultiCartService },
{ provide: UserIdService, useClass: MockUserIdService },
{ provide: FeatureConfigService, useClass: MockFeatureConfigService },
],
});
}
Expand Down Expand Up @@ -397,6 +408,9 @@ describe('CartItemListComponent', () => {

describe('Use outlet with outlet context data', () => {
it('should be able to get inputs from outlet context data', () => {
const newContext = structuredClone(mockContext);
newContext.items = [mockItem0];
const context$ = new Subject();
configureTestingModule().overrideProvider(OutletContextData, {
useValue: { context$ },
});
Expand All @@ -406,18 +420,16 @@ describe('CartItemListComponent', () => {
spyOn(<any>component, '_setItems').and.callThrough();
const setLoading = spyOnProperty(component, 'setLoading', 'set');
component.ngOnInit();

expect(component.cartId).toEqual(mockContext.cartId);
expect(component.hasHeader).toEqual(mockContext.hasHeader);
expect(component['_setItems']).toHaveBeenCalledWith(mockContext.items, {
forceRerender: false,
context$.next(newContext);
expect(component.cartId).toEqual(newContext.cartId);
expect(component.hasHeader).toEqual(newContext.hasHeader);
expect(component['_setItems']).toHaveBeenCalledWith(newContext.items, {
forceRerender: true,
});
expect(component.options).toEqual(mockContext.options);
expect(component.promotionLocation).toEqual(
mockContext.promotionLocation
);
expect(component.readonly).toEqual(mockContext.readonly);
expect(setLoading).toHaveBeenCalledWith(mockContext.cartIsLoading);
expect(component.options).toEqual(newContext.options);
expect(component.promotionLocation).toEqual(newContext.promotionLocation);
expect(component.readonly).toEqual(newContext.readonly);
expect(setLoading).toHaveBeenCalledWith(newContext.cartIsLoading);
});

it('should mark view for check and force re-creation of item controls when outlet context emits with changed read-only flag', () => {
Expand All @@ -443,5 +455,19 @@ describe('CartItemListComponent', () => {
component.form.get(mockItem1.entryNumber.toString())
);
});

it('should not call _setItems when neither contextRequiresRerender nor isItemsChanged are true', () => {
configureTestingModule().overrideProvider(OutletContextData, {
useValue: { context$ },
});
TestBed.compileComponents();
stubSeviceAndCreateComponent();

spyOn(<any>component, '_setItems').and.callThrough();
component.ngOnInit();

// context$ emits mockContext which has the same items that were set in stubSeviceAndCreateComponent
expect(component['_setItems']).not.toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
OnDestroy,
OnInit,
Optional,
inject,
} from '@angular/core';
import { UntypedFormControl, UntypedFormGroup } from '@angular/forms';
import {
Expand All @@ -24,7 +25,7 @@ import {
SelectiveCartFacade,
CartOutlets,
} from '@spartacus/cart/base/root';
import { UserIdService } from '@spartacus/core';
import { FeatureConfigService, UserIdService } from '@spartacus/core';
import { OutletContextData } from '@spartacus/storefront';
import { Observable, Subscription } from 'rxjs';
import { map, startWith, tap } from 'rxjs/operators';
Expand Down Expand Up @@ -84,6 +85,7 @@ export class CartItemListComponent implements OnInit, OnDestroy {
}
}
readonly CartOutlets = CartOutlets;
private featureConfigService = inject(FeatureConfigService);
constructor(
protected activeCartService: ActiveCartFacade,
protected selectiveCartService: SelectiveCartFacade,
Expand Down Expand Up @@ -133,15 +135,35 @@ export class CartItemListComponent implements OnInit, OnDestroy {
if (context.cartIsLoading !== undefined) {
this.setLoading = context.cartIsLoading;
}
if (context.items !== undefined) {
this.cd.markForCheck();
this._setItems(context.items, {
forceRerender: contextRequiresRerender,
});
}
this.updateItemsOnContextChange(context, contextRequiresRerender);
});
}

protected updateItemsOnContextChange(
context: ItemListContext,
contextRequiresRerender: boolean
) {
const preventRedundantRecreationEnabled =
this.featureConfigService.isEnabled(
'a11yPreventCartItemsFormRedundantRecreation'
);
if (
context.items !== undefined &&
(!preventRedundantRecreationEnabled ||
contextRequiresRerender ||
this.isItemsChanged(context.items))
) {
this.cd.markForCheck();
this._setItems(context.items, {
forceRerender: contextRequiresRerender,
});
}
}

protected isItemsChanged(newItems: OrderEntry[]): boolean {
return JSON.stringify(this.items) !== JSON.stringify(newItems);
}

/**
* Resolves items passed to component input and updates 'items' field
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,12 @@ export interface FeatureTogglesInterface {
*/
a11ySemanticPaginationLabel?: boolean;

/**
* When using CartItemListComponent as an outlet ([cxOutlet]="CartOutlets.CART_ITEM_LIST"):
* prevents the form from being recreated when neither the items nor other dependent properties (e.g., readonly) have changed.
*/
a11yPreventCartItemsFormRedundantRecreation?: boolean;

/**
* Prevents screen reader from stopping on invisible elements when being in read mode for `BreadcrumbComponent`, `QuickOrderFormComponent`
*/
Expand Down Expand Up @@ -399,6 +405,7 @@ export const defaultFeatureToggles: Required<FeatureTogglesInterface> = {
a11yVisibleFocusOverflows: false,
a11yTruncatedTextForResponsiveView: false,
a11ySemanticPaginationLabel: false,
a11yPreventCartItemsFormRedundantRecreation: false,
a11yPreventSRFocusOnHiddenElements: false,
a11yMyAccountLinkOutline: false,
a11yCloseProductImageBtnFocus: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ if (environment.estimatedDeliveryDate) {
a11yVisibleFocusOverflows: true,
a11yTruncatedTextForResponsiveView: true,
a11ySemanticPaginationLabel: true,
a11yPreventCartItemsFormRedundantRecreation: true,
a11yMyAccountLinkOutline: true,
a11yCloseProductImageBtnFocus: true,
a11yNotificationPreferenceFieldset: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ describe('ItemCounterComponent', () => {
input.dispatchEvent(new KeyboardEvent('keyup', { key: 'Enter' }));
fixture.detectChanges();

expect(component.control.value).toEqual('10');
expect(component.control.value).toEqual(10);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ export class ItemCounterComponent implements OnInit, OnDestroy {
* It is used to improve keyboard controls of the component.
*/
updateValue(): void {
this.control.setValue(this.input.nativeElement.value);
// Convert string value to number(prevents us from having '1' + 1 = '11' on increment)
this.control.setValue(+this.input.nativeElement.value);
this.control.markAsDirty();
}

Expand Down

0 comments on commit 94926b6

Please sign in to comment.