Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(*): Remove the need of hardcoded item heights in combo #14871

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ export class IgxComboDropDownComponent extends IgxDropDownComponent implements I

public override ngAfterViewInit() {
this.virtDir.getScroll().addEventListener('scroll', this.scrollHandler);
this.toggleDirective?.animationStarting.subscribe((_args: any) => {
this.animationStarting.emit(_args);
});
}

/**
Expand Down
50 changes: 21 additions & 29 deletions projects/igniteui-angular/src/lib/combo/combo.common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ import { IComboItemAdditionEvent, IComboSearchInputEventArgs } from './public_ap
import { ComboResourceStringsEN, IComboResourceStrings } from '../core/i18n/combo-resources';
import { getCurrentResourceStrings } from '../core/i18n/resources';
import { DOCUMENT } from '@angular/common';
import { Size } from '../grids/common/enums';
import { isEqual } from 'lodash-es';
import { ToggleViewEventArgs } from '../directives/toggle/toggle.directive';

export const IGX_COMBO_COMPONENT = /*@__PURE__*/new InjectionToken<IgxComboBase>('IgxComboComponentToken');

Expand Down Expand Up @@ -82,19 +82,6 @@ export interface IgxComboBase {

let NEXT_ID = 0;

/**
* @hidden
* The default number of items that should be in the combo's
* drop-down list if no `[itemsMaxHeight]` is specified
*/
const itemsInContainer = 10; // TODO: make private readonly

/** @hidden @internal */
const ItemHeights = {
"3": 40,
"2": 32,
"1": 28
};

/** @hidden @internal */
export const enum DataTypes {
Expand Down Expand Up @@ -234,8 +221,8 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
*/
@Input()
public get itemsMaxHeight(): number {
if (this._itemsMaxHeight === null || this._itemsMaxHeight === undefined) {
return this.itemHeight * itemsInContainer;
if (this.itemHeight && !this._itemsMaxHeight) {
return this.itemHeight * this.itemsInCointaner;
}
return this._itemsMaxHeight;
}
Expand All @@ -246,15 +233,9 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh

/** @hidden */
public get itemsMaxHeightInRem() {
return rem(this.itemsMaxHeight);
}

/**
* @hidden
* @internal
*/
public get comboSize(): Size {
return this.computedStyles?.getPropertyValue('--ig-size') || Size.Large;
if (this.itemsMaxHeight) {
return rem(this.itemsMaxHeight);
}
}

/**
Expand All @@ -272,9 +253,6 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
*/
@Input()
public get itemHeight(): number {
if (this._itemHeight === null || this._itemHeight === undefined) {
return ItemHeights[this.comboSize];
}
return this._itemHeight;
}

Expand Down Expand Up @@ -943,6 +921,9 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
public set filteringOptions(value: IComboFilteringOptions) {
this._filteringOptions = value;
}

protected containerSize = undefined;
protected itemSize = undefined;
protected _data = [];
protected _value = [];
protected _displayValue = '';
Expand All @@ -963,12 +944,13 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
private _id: string = `igx-combo-${NEXT_ID++}`;
private _type = null;
private _dataType = '';
private _itemHeight = null;
private _itemHeight = undefined;
private _itemsMaxHeight = null;
private _overlaySettings: OverlaySettings;
private _groupSortingDirection: SortingDirection = SortingDirection.Asc;
private _filteringOptions: IComboFilteringOptions;
private _defaultFilteringOptions: IComboFilteringOptions = { caseSensitive: false };
private itemsInCointaner = 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo -> itemsInContainer :)


public abstract dropdown: IgxComboDropDownComponent;
public abstract selectionChanging: EventEmitter<any>;
Expand Down Expand Up @@ -1029,6 +1011,16 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
const eventArgs: IForOfState = Object.assign({}, e, { owner: this });
this.dataPreLoad.emit(eventArgs);
});
this.dropdown?.animationStarting.subscribe((_args: ToggleViewEventArgs) => {
// calculate the container size and item size based on the sizes from the DOM
const dropdownContainerHeight = this.dropdownContainer.nativeElement.getBoundingClientRect().height;
if (dropdownContainerHeight) {
this.containerSize = parseFloat(dropdownContainerHeight);
}
if (this.dropdown.children?.first) {
this.itemSize = this.dropdown.children.first.element.nativeElement.getBoundingClientRect().height;
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so this is happening because the overlay opening is emitted before updating sizes, which I realize is likely to check for cancel and avoid doing redundant work. The issue is animationStarting is not guaranteed:

if (info.settings.positionStrategy.settings.openAnimation) {
// TODO: should we build players again. This was already done in attach!!!
// this.buildAnimationPlayers(info);
this.playOpenAnimation(info);

As we discussed, the things updateSize can probably be performed in advance without causing trouble, though I'll tag @wnvko in case he can recall reasons not to.

Alternatively, the combo itself can patch opening and force the update sooner to be able to calculate those on opening.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it's also quite possible we could move the --ig-size transfer on attach entirely, since that doesn't change after the fact and it's applied to the content container, if the other stuff are not safe to move.
That's all to avoid both relying on animationStarting and propagating it across components :)

}

/** @hidden @internal */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
<igx-combo-item [itemHeight]="itemHeight" *igxFor="let item of data
damyanpetev marked this conversation as resolved.
Show resolved Hide resolved
| comboFiltering:filterValue:displayKey:filteringOptions:filterFunction:disableFiltering
| comboGrouping:groupKey:valueKey:groupSortingDirection:compareCollator;
index as rowIndex; containerSize: itemsMaxHeight; scrollOrientation: 'vertical'; itemSize: itemHeight"
index as rowIndex; initialChunkSize: 10; containerSize: itemsMaxHeight || containerSize; itemSize: itemHeight || itemSize, scrollOrientation: 'vertical';"
[value]="item" [isHeader]="item?.isHeader" [index]="rowIndex" [role]="item?.isHeader? 'group' : 'option'">
<ng-container *ngIf="item?.isHeader">
<ng-container
Expand Down
28 changes: 3 additions & 25 deletions projects/igniteui-angular/src/lib/combo/combo.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1025,8 +1025,6 @@ describe('igxCombo', () => {
expect(combo.displayKey).toEqual('field');
expect(combo.groupKey).toEqual('region');
expect(combo.width).toEqual('400px');
expect(combo.itemsMaxHeight).toEqual(320);
expect(combo.itemHeight).toEqual(32);
expect(combo.placeholder).toEqual('Location');
expect(combo.disableFiltering).toEqual(false);
expect(combo.allowCustomValues).toEqual(false);
Expand Down Expand Up @@ -1125,9 +1123,7 @@ describe('igxCombo', () => {
const dropdownList = fixture.debugElement.query(By.css(`.${CSS_CLASS_CONTENT}`));

const verifyDropdownItemHeight = () => {
expect(combo.itemHeight).toEqual(itemHeight);
expect(dropdownItems[0].nativeElement.clientHeight).toEqual(itemHeight);
expect(combo.itemsMaxHeight).toEqual(itemMaxHeight);
expect(dropdownList.nativeElement.clientHeight).toEqual(itemMaxHeight);
};
verifyDropdownItemHeight();
Expand Down Expand Up @@ -1764,11 +1760,10 @@ describe('igxCombo', () => {
});
it('should focus item when onFocus and onBlur are called', () => {
expect(dropdown.focusedItem).toEqual(null);
expect(dropdown.items.length).toEqual(9);
dropdown.toggle();
fixture.detectChanges();
expect(dropdown.items).toBeDefined();
expect(dropdown.items.length).toBeTruthy();
expect(dropdown.items.length).toEqual(9);
dropdown.onFocus();
expect(dropdown.focusedItem).toEqual(dropdown.items[0]);
expect(dropdown.focusedItem.focused).toEqual(true);
Expand Down Expand Up @@ -2015,7 +2010,8 @@ describe('igxCombo', () => {
expect(combo.dropdown.onToggleOpened).toHaveBeenCalledTimes(1);
let vContainerScrollHeight = virtDir.getScroll().scrollHeight;
expect(virtDir.getScroll().scrollTop).toEqual(0);
expect(vContainerScrollHeight).toBeGreaterThan(combo.itemHeight);
const itemHeight = parseFloat(combo.dropdown.children.first.element.nativeElement.getBoundingClientRect().height);
expect(vContainerScrollHeight).toBeGreaterThan(itemHeight);
virtDir.getScroll().scrollTop = Math.floor(vContainerScrollHeight / 2);
await firstValueFrom(combo.virtualScrollContainer.chunkLoad);
fixture.detectChanges();
Expand Down Expand Up @@ -3573,24 +3569,6 @@ describe('igxCombo', () => {
}));
});
});
describe('Display density', () => {
beforeEach(() => {
fixture = TestBed.createComponent(IgxComboSampleComponent);
fixture.detectChanges();
combo = fixture.componentInstance.combo;
});
it('should scale items container depending on size (itemHeight * 10)', () => {
combo.toggle();
fixture.detectChanges();
expect(combo.itemsMaxHeight).toEqual(320);
fixture.componentInstance.size = 'small';
fixture.detectChanges();
expect(combo.itemsMaxHeight).toEqual(280);
fixture.componentInstance.size = 'large';
fixture.detectChanges();
expect(combo.itemsMaxHeight).toEqual(400);
});
});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@
%igx-combo__content {
position: relative;
overflow: hidden;
max-height: calc(var(--size) * 10);

&:focus {
outline: transparent;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,14 @@ export class IgxForOfDirective<T, U extends T[] = T[]> extends IgxForOfToken<T,U
@Input()
public igxForContainerSize: any;

/**
* @hidden
* @internal
* Initial chunk size if no container size is passed. If container size is passed then the igxForOf calculates its chunk size
*/
@Input()
public igxForInitialChunkSize: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess existing combo tests somewhat cover the use of igxForInitialChunkSize but it wouldn't hurt to have at least one unit test covering this just for the for-of


/**
* Sets the px-affixed size of the item along the axis of scrolling.
* For "horizontal" orientation this value is the width of the column and for "vertical" is the height or the row.
Expand Down Expand Up @@ -1207,7 +1215,7 @@ export class IgxForOfDirective<T, U extends T[] = T[]> extends IgxForOfToken<T,U
}
} else {
if (this.igxForOf) {
chunkSize = this.igxForOf.length;
chunkSize = Math.min(this.igxForInitialChunkSize || this.igxForOf.length, this.igxForOf.length);
}
}
return chunkSize;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,13 @@ export class IgxToggleDirective implements IToggleView, OnInit, OnDestroy {
@Output()
public appended = new EventEmitter<ToggleViewEventArgs>();

/**
* @hidden @internal
* Emitted just before the overlay animation start.
*/
@Output()
public animationStarting = new EventEmitter();

/**
* @hidden
*/
Expand Down Expand Up @@ -193,6 +200,7 @@ export class IgxToggleDirective implements IToggleView, OnInit, OnDestroy {
private _overlayClosingSub: Subscription;
private _overlayClosedSub: Subscription;
private _overlayContentAppendedSub: Subscription;
private _overlayAnimationStartingSub: Subscription;

/**
* @hidden
Expand Down Expand Up @@ -387,13 +395,18 @@ export class IgxToggleDirective implements IToggleView, OnInit, OnDestroy {
.closed
.pipe(...this._overlaySubFilter)
.subscribe(this.overlayClosed);

this._overlayAnimationStartingSub = this.overlayService.animationStarting.pipe(first(), takeUntil(this.destroy$)).subscribe(() => {
this.animationStarting.emit();
});
}

private unsubscribe() {
this.clearSubscription(this._overlayOpenedSub);
this.clearSubscription(this._overlayClosingSub);
this.clearSubscription(this._overlayClosedSub);
this.clearSubscription(this._overlayContentAppendedSub);
this.clearSubscription(this._overlayAnimationStartingSub);
}

private clearSubscription(subscription: Subscription) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ export class IgxDropDownComponent extends IgxDropDownBaseDirective implements ID
@Output()
public closed = new EventEmitter<IBaseEventArgs>();

/**
* @hidden
* @internal
* Emitted just before the overlay animation start.
*/
@Output()
public animationStarting = new EventEmitter<ToggleViewEventArgs>();

/**
* Gets/sets whether items take focus. Disabled by default.
* When enabled, drop down items gain tab index and are focused when active -
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
[itemHeight]="itemHeight" (click)="handleItemClick()" *igxFor="let item of data
| comboFiltering:filterValue:displayKey:filteringOptions:filterFunction
| comboGrouping:groupKey:valueKey:groupSortingDirection:compareCollator;
index as rowIndex; containerSize: itemsMaxHeight; scrollOrientation: 'vertical'; itemSize: itemHeight"
index as rowIndex; initialChunkSize: 10; containerSize: itemsMaxHeight || containerSize; itemSize: itemHeight || itemSize; scrollOrientation: 'vertical';"
[value]="item" [isHeader]="item?.isHeader" [index]="rowIndex">
<ng-container *ngIf="item?.isHeader">
<ng-container
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -588,8 +588,6 @@ describe('IgxSimpleCombo', () => {
expect(combo.displayKey).toEqual('field');
expect(combo.groupKey).toEqual('region');
expect(combo.width).toEqual('400px');
expect(combo.itemsMaxHeight).toEqual(320);
expect(combo.itemHeight).toEqual(32);
expect(combo.placeholder).toEqual('Location');
expect(combo.allowCustomValues).toEqual(false);
expect(combo.cssClass).toEqual(CSS_CLASS_COMBO);
Expand Down Expand Up @@ -671,9 +669,7 @@ describe('IgxSimpleCombo', () => {
const dropdownList = fixture.debugElement.query(By.css(`.${CSS_CLASS_CONTENT}`));

const verifyDropdownItemHeight = () => {
expect(combo.itemHeight).toEqual(itemHeight);
expect(dropdownItems[0].nativeElement.clientHeight).toEqual(itemHeight);
expect(combo.itemsMaxHeight).toEqual(itemMaxHeight);
expect(dropdownList.nativeElement.clientHeight).toEqual(itemMaxHeight);
};
verifyDropdownItemHeight();
Expand Down Expand Up @@ -2115,35 +2111,6 @@ describe('IgxSimpleCombo', () => {
}));
});

describe('Display density', () => {
beforeAll(waitForAsync(() => {
TestBed.configureTestingModule({
imports: [
NoopAnimationsModule,
ReactiveFormsModule,
FormsModule,
IgxSimpleComboSampleComponent
]
}).compileComponents();
}));
beforeEach(() => {
fixture = TestBed.createComponent(IgxSimpleComboSampleComponent);
fixture.detectChanges();
combo = fixture.componentInstance.combo;
});
it('should scale items container depending on component size (itemHeight * 10)', () => {
combo.toggle();
fixture.detectChanges();
expect(combo.itemsMaxHeight).toEqual(320);
fixture.componentInstance.size = 'small';
fixture.detectChanges();
expect(combo.itemsMaxHeight).toEqual(280);
fixture.componentInstance.size = 'large';
fixture.detectChanges();
expect(combo.itemsMaxHeight).toEqual(400);
});
});

describe('Form control tests: ', () => {
describe('Template form tests: ', () => {
let inputGroupRequired: DebugElement;
Expand Down