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

fix(core): address Avatar Group issues #12530

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
22be6e6
fix(core): avatar group bugs
khotcholava Sep 25, 2024
4a87b4d
fix(core): avatar group bugs
khotcholava Sep 26, 2024
b39b871
fix(core): avatar group
khotcholava Sep 30, 2024
aedf05e
Merge branch 'main' into fix(core)-avatar-group-bugs
khotcholava Oct 10, 2024
b32ae7e
fix(core): Resolve focus retention & rendering issues, add feature en…
khotcholava Oct 10, 2024
fe4920e
fix(core): Resolve focus retention & rendering issues, add feature en…
khotcholava Oct 10, 2024
0ead287
Make avatar group circle
khotcholava Oct 11, 2024
1f11a17
Make avatar group circle
khotcholava Oct 11, 2024
da922ad
Implemented first selection of popover avatar items
khotcholava Oct 14, 2024
f587f50
fix(core): avatar group
khotcholava Oct 15, 2024
4373cde
fix(core): Resolve focus retention & rendering issues, add feature en…
khotcholava Oct 25, 2024
b6af950
Merge branch 'main' into fix(core)-avatar-group-bugs
khotcholava Oct 29, 2024
3768cb9
Merge branch 'main' into fix(core)-avatar-group-bugs
khotcholavaSuzy Nov 7, 2024
f79c75e
fix(core): avatar groups
khotcholavaSuzy Nov 7, 2024
3405d09
fix(core): avatar groups
khotcholavaSuzy Nov 7, 2024
3a61be4
fix(core): avatar groups
khotcholavaSuzy Nov 7, 2024
8b1e6de
fix(core): avatar groups
khotcholavaSuzy Nov 8, 2024
8251f84
Merge branch 'main' into fix(core)-avatar-group-bugs
mikerodonnell89 Nov 12, 2024
d20946e
fix(core): avatar groups
khotcholavaSuzy Nov 13, 2024
cb1c93d
fix(core): avatar group fixed first item focus
khotcholavaSuzy Nov 27, 2024
d353a20
fix(core): avatar group fixed first item focus
khotcholavaSuzy Nov 27, 2024
18fc966
fix e2e test
khotcholava Dec 2, 2024
827ef21
Fixed popover focus
khotcholava Dec 2, 2024
d4bd252
fix e2e test
khotcholava Dec 2, 2024
bc16f40
Merge remote-tracking branch 'origin/fix(core)-avatar-group-bugs' int…
khotcholava Dec 2, 2024
728a86b
fixed time picker tests
khotcholava Dec 3, 2024
87c2469
fixed time picker tests
khotcholava Dec 3, 2024
fada1bc
Merge branch 'main' into fix(core)-avatar-group-bugs
khotcholava Dec 20, 2024
3420564
Merge branch 'main' into fix(core)-avatar-group-bugs
mikerodonnell89 Jan 2, 2025
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
16 changes: 13 additions & 3 deletions libs/core/avatar-group/avatar-group.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
(resized)="_detectChanges()"
>
@for (item of _avatars; track item) {
<fd-popover [noArrow]="false" [focusAutoCapture]="true">
<fd-popover [placement]="popoverPlacement" [noArrow]="false" [focusAutoCapture]="true">
<fd-popover-control tabindex="-1" (click)="outletItem.element.focus()">
<ng-template
#outletItem="fdAvatarGroupItemPortal"
Expand All @@ -33,8 +33,10 @@
</fd-popover>
}
<fd-popover
[placement]="popoverPlacement"
[noArrow]="false"
[focusAutoCapture]="true"
(isOpenChange)="handlePopoverOpen($event)"
*fdAvatarGroupInternalOverflowButton="avatarGroupHostComponent._hiddenItems(); let hiddenItems"
>
<fd-popover-control tabindex="-1">
Expand All @@ -52,9 +54,17 @@
</fd-popover>
</fd-avatar-group-host>
} @else {
<fd-popover [noArrow]="false" [focusAutoCapture]="true" fdkResizeObserver (resized)="_detectChanges()">
<fd-popover-control>
<fd-popover
[placement]="popoverPlacement"
[noArrow]="false"
[focusAutoCapture]="false"
fdkResizeObserver
(resized)="_detectChanges()"
(isOpenChange)="handlePopoverOpen($event)"
>
<fd-popover-control tabindex="-1">
<fd-avatar-group-host
[maxVisibleItems]="maxVisibleItems"
#avatarGroupHostComponent
[type]="type"
[size]="size"
Expand Down
44 changes: 40 additions & 4 deletions libs/core/avatar-group/avatar-group.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,25 @@ import {
ContentChild,
ContentChildren,
Input,
OnInit,
QueryList,
ViewChild,
ViewChildren,
ViewEncapsulation,
computed,
inject
inject,
signal
} from '@angular/core';
import {
DynamicPortalComponent,
FocusableItemDirective,
FocusableListDirective,
Nullable,
ResizeObserverDirective,
RtlService
} from '@fundamental-ngx/cdk/utils';
import { PopoverModule } from '@fundamental-ngx/core/popover';
import { PopoverModule, PopoverService } from '@fundamental-ngx/core/popover';
import { Placement } from '@fundamental-ngx/core/shared';
import { AvatarGroupHostComponent } from './components/avatar-group-host.component';
import { AvatarGroupOverflowButtonComponent } from './components/avatar-group-overflow-button.component';
import { DefaultAvatarGroupOverflowBodyComponent } from './components/default-avatar-group-overflow-body/default-avatar-group-overflow-body.component';
Expand All @@ -44,7 +49,8 @@ import { AvatarGroupHostConfig } from './types';
{
provide: AVATAR_GROUP_HOST_CONFIG,
useExisting: AvatarGroupComponent
}
},
PopoverService
],
imports: [
AvatarGroupHostComponent,
Expand All @@ -61,7 +67,7 @@ import { AvatarGroupHostConfig } from './types';
ResizeObserverDirective
]
})
export class AvatarGroupComponent implements AvatarGroupHostConfig {
export class AvatarGroupComponent implements AvatarGroupHostConfig, OnInit {
/**
* The AvatarGroup control has two group types:
*
Expand All @@ -88,6 +94,10 @@ export class AvatarGroupComponent implements AvatarGroupHostConfig {
@Input()
size: AvatarGroupHostConfig['size'] = 'l';

/** Popover placement */
@Input()
popoverPlacement: Placement | null = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

use Nullable type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PopoverPlacement type is Placement | null. It's not Nullable. If I change it I'm afraid it will broke already existing functionality


/**
* The title which is displayed when user opens the overflow popover.
* This takes effect only when default overflow popover body is used,
Expand All @@ -96,10 +106,20 @@ export class AvatarGroupComponent implements AvatarGroupHostConfig {
@Input()
overflowPopoverTitle: string;

/**
* The maximum number of visible avatar items.
**/
@Input()
maxVisibleItems: Nullable<number> = null;

/** @hidden */
@ViewChildren(AvatarGroupItemRendererDirective)
_avatarRenderers: QueryList<AvatarGroupItemRendererDirective>;

/** @hidden */
@ViewChild(DefaultAvatarGroupOverflowBodyComponent)
defaultAvatarGroupOverflowBody: DefaultAvatarGroupOverflowBodyComponent;

/** @hidden */
@ContentChildren(AvatarGroupItemDirective)
_avatars: QueryList<AvatarGroupItemDirective>;
Expand All @@ -112,6 +132,9 @@ export class AvatarGroupComponent implements AvatarGroupHostConfig {
@ContentChild(AvatarGroupOverflowBodyDirective)
_avatarGroupPopoverBody: AvatarGroupOverflowBodyDirective;

/** @hidden */
opened = signal(false);

/** @hidden */
_contentDirection$ = computed<Direction>(() => (this._rtlService?.rtlSignal() ? 'rtl' : 'ltr'));

Expand All @@ -121,6 +144,19 @@ export class AvatarGroupComponent implements AvatarGroupHostConfig {
/** @hidden */
private readonly _rtlService = inject(RtlService, { optional: true });

/** @hidden */
private _popoverService = inject(PopoverService);

/** @hidden */
ngOnInit(): void {
this._popoverService._forceFocus.set(true);
}

/** @hidden */
handlePopoverOpen($event: boolean): void {
this.opened.set($event);
}

/** @hidden */
_detectChanges(): void {
this._cdr.detectChanges();
Expand Down
14 changes: 13 additions & 1 deletion libs/core/avatar-group/components/avatar-group-host.component.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

add or update relevant unit tests for maxVisibleItems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not e2e test for avatar group host

Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ export class AvatarGroupHostComponent
@Input()
items: QueryList<AvatarGroupItemDirective>;

/**
* The maximum number of visible avatar items.
**/
@Input()
maxVisibleItems: Nullable<number> = null;

/**
* @hidden
* The portals to be rendered in the avatar group.
Expand Down Expand Up @@ -166,12 +172,18 @@ export class AvatarGroupHostComponent
continue;
}
accWidth += item.width;
if (accWidth <= containerWidth) {
if (accWidth <= containerWidth && (!this.maxVisibleItems || visibleItems.length < this.maxVisibleItems)) {
visibleItems.push(item);
} else if (!item.forceVisibility) {
hiddenItems.push(item);
}
}

// Ensure we don't exceed the maxVisibleItems limit
if (this.maxVisibleItems && visibleItems.length > this.maxVisibleItems) {
hiddenItems.unshift(...visibleItems.splice(this.maxVisibleItems));
}

/* take last item from the visibleItems which is not forced to be visible and push it to the hiddenItems
* This is done to free up the space for the overflow button
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {
ChangeDetectionStrategy,
Component,
forwardRef,
HostBinding,
inject,
Input,
OnChanges,
Expand All @@ -29,7 +28,8 @@ import { AVATAR_GROUP_HOST_CONFIG } from '../tokens';
selector: 'fd-avatar-group-overflow-button',
template: ` <ng-content></ng-content>`,
host: {
role: 'button'
role: 'button',
class: 'fd-avatar--circle'
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand All @@ -51,13 +51,6 @@ export class AvatarGroupOverflowButtonComponent
@Input()
size: Size = 'l';

/**
* Whether the overflow button should be displayed as a circle.
*/
@Input()
@HostBinding('class.fd-avatar--circle')
circle = false;

/**
* A number from 1 to 10 representing the background color of the Avatar.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@
navigationDirection="horizontal"
>
@if (_overflowPopoverStage === 'main') {
@for (item of avatars; track item) {
@for (avatar of avatars(); track avatar) {
<ng-template
cdkPortalOutlet
fdAvatarGroupItemPortal
[avatarGroupItem]="item.avatarGroupItem"
[avatarGroupItem]="avatar.avatarGroupItem"
[forceVisibility]="true"
>
</ng-template>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ import {
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
EventEmitter,
Input,
OnDestroy,
Output,
QueryList,
Renderer2,
ViewChildren,
ViewEncapsulation,
inject
inject,
input
} from '@angular/core';
import { FocusableListDirective, RtlService, elementClick$ } from '@fundamental-ngx/cdk/utils';
import { BarModule } from '@fundamental-ngx/core/bar';
Expand Down Expand Up @@ -42,18 +45,16 @@ import { AvatarGroupItemDirective } from '../../directives/avatar-group-item.dir
changeDetection: ChangeDetectionStrategy.OnPush
})
export class DefaultAvatarGroupOverflowBodyComponent implements AfterViewInit, OnDestroy {
/**
* List of avatars to be rendered in the overflow popover.
**/
@Input()
avatars: Iterable<AvatarGroupItemRendererDirective> = [];

/**
* Title of the overflow popover.
* */
@Input()
overflowPopoverTitle: string;

/** @hidden */
@Output()
back = new EventEmitter<void>();

/** @hidden */
@ViewChildren(AvatarGroupItemRendererDirective)
_avatarGroupItemPortals: QueryList<AvatarGroupItemRendererDirective>;
Expand All @@ -73,6 +74,12 @@ export class DefaultAvatarGroupOverflowBodyComponent implements AfterViewInit, O
get isRtl(): boolean {
return !!this._rtlService?.rtl.value;
}

/**
* List of avatars to be rendered in the overflow popover.
**/
avatars = input<AvatarGroupItemRendererDirective[]>([]);

/** @hidden */
private _itemClickSubscription: Subscription;

Expand Down Expand Up @@ -122,5 +129,6 @@ export class DefaultAvatarGroupOverflowBodyComponent implements AfterViewInit, O
_openOverflowMain(): void {
this._overflowPopoverStage = 'main';
this._changeDetectorRef.detectChanges();
this.back.emit();
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { CdkPortalOutlet, TemplatePortal } from '@angular/cdk/portal';
import { Directive, EmbeddedViewRef, Input, OnInit, ViewContainerRef, inject } from '@angular/core';
import { Directive, EmbeddedViewRef, Input, OnInit, ViewContainerRef, effect, inject } from '@angular/core';
import { FDK_FOCUSABLE_ITEM_DIRECTIVE, FocusableItem, Nullable } from '@fundamental-ngx/cdk/utils';
import { AvatarGroupComponent } from '@fundamental-ngx/core/avatar-group';
import { BehaviorSubject, Observable, fromEvent } from 'rxjs';
import { filter, switchMap } from 'rxjs/operators';
import { AVATAR_GROUP_HOST_CONFIG } from '../tokens';
Expand Down Expand Up @@ -118,6 +119,11 @@ export class AvatarGroupItemRendererDirective implements OnInit, FocusableItem {
filter((element) => !!element),
switchMap((element) => fromEvent(element as unknown as HTMLElement, 'keydown') as Observable<KeyboardEvent>)
);
effect(() => {
const config = this._hostConfig as AvatarGroupComponent;
this.setTabbable(config.type === 'individual' || config.opened());
this._isFocusable = config.type === 'individual' || config.opened();
});
}

/** @hidden */
Expand Down Expand Up @@ -147,8 +153,6 @@ export class AvatarGroupItemRendererDirective implements OnInit, FocusableItem {
this._embeddedViewRef = this._portalOutlet.attach(this._templatePortal);
this._embeddedViewRef.detectChanges();
this._element$.next(this.element);
this.setTabbable(this._hostConfig.type === 'individual');
this._isFocusable = this._hostConfig.type === 'individual';
}

/**
Expand Down
5 changes: 3 additions & 2 deletions libs/core/popover/popover-body/popover-body.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,10 @@ export class PopoverBodyComponent implements AfterViewInit {
@HostListener('document:click', ['$event.target'])
onClick(targetElement: HTMLElement): void {
const clickedInside = this._elementRef.nativeElement.contains(targetElement);
if (!clickedInside) {
// Call the focus logic if clicked outside the popover
if (!clickedInside && this._focusAutoCapture) {
this._focusFirstTabbableElement();
} else {
this._focusFirstTabbableElement(true);
}
}

Expand Down
22 changes: 19 additions & 3 deletions libs/core/popover/popover-service/popover.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import {
Renderer2,
TemplateRef,
ViewContainerRef,
inject
inject,
signal
} from '@angular/core';
import { Observable, Subject, merge } from 'rxjs';
import { distinctUntilChanged, filter, startWith, takeUntil } from 'rxjs/operators';
Expand Down Expand Up @@ -46,6 +47,9 @@ export class PopoverService extends BasePopoverClass {
/** Template content displayed inside popover body */
templateContent: Nullable<TemplateRef<any>>;

/** Whether to focus the first item on space key press */
_forceFocus = signal(false);

/** @hidden */
_onLoad = new Subject<ElementRef>();

Expand Down Expand Up @@ -127,6 +131,13 @@ export class PopoverService extends BasePopoverClass {
this._removeOverlay(this._modalBodyClass, this._modalTriggerClass);
}
});

// Add keydown listener for space key
this._renderer.listen('document', 'keydown', (event: KeyboardEvent) => {
Copy link
Contributor

@dpavlenishvili dpavlenishvili Dec 2, 2024

Choose a reason for hiding this comment

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

Listening for keydown events globally on the document level can lead some performance issues, if it is possible scope the event listener to the popover element directly.
for example:
`
const popoverElement = this._elementRef.nativeElement;

this._renderer.listen(popoverElement, 'keydown', (event: KeyboardEvent) => {
if (event.code === 'Space' && this.isOpen) {
this._storeAndFocusFirstTabbable();
}
});
`

if (event.code === 'Space' && this.isOpen) {
this._storeAndFocusFirstTabbable();
}
});
}

/**
Expand Down Expand Up @@ -588,11 +599,16 @@ export class PopoverService extends BasePopoverClass {
/** @hidden */
private _focusFirstTabbableElement(focusLastElement = true): void {
if (focusLastElement && this.focusAutoCapture) {
this._lastActiveElement = <HTMLElement>document.activeElement;
this._getPopoverBody()?._focusFirstTabbableElement();
this._storeAndFocusFirstTabbable();
}
}

/** @hidden */
private _storeAndFocusFirstTabbable(): void {
this._lastActiveElement = <HTMLElement>document.activeElement;
this._getPopoverBody()?._focusFirstTabbableElement(true);
}

/** @hidden */
private _focusLastActiveElementBeforeOpen(focusLastElement = true): void {
if (focusLastElement && this.restoreFocusOnClose && this.focusAutoCapture && this._lastActiveElement) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
<fd-avatar-group-default-example></fd-avatar-group-default-example>
</component-example>
<code-example [exampleFiles]="avatarGroupDefaultExample"></code-example>

<fd-docs-section-title id="group" componentName="AvatarGroup"> Group Type Standard Usage </fd-docs-section-title>
<description>
<p>
Expand Down
Loading
Loading