-
Notifications
You must be signed in to change notification settings - Fork 132
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
base: main
Are you sure you want to change the base?
Conversation
- Fixed focus on avatar group
…hancements Closes [#12369](#12369) ## Description - Fixed focus bug where focus remained on the first avatar group control after interacting with the second one. - Fixed rendering issue where only "+n more" button displayed after switching tabs instead of the full avatar group row. - Added feature to open group popover from the center of the avatar group. - Added configuration option to make "+n more" button circular to match avatars typically being in circle shapes. - Introduced property to set the number of visible avatars before displaying the "+n more" button.
❌ Check the deploy log for errors here: https://app.netlify.com/sites/fundamental-ngx/deploys
|
Visit the preview URL for this PR (updated for commit 3420564): https://fundamental-ngx-gh--pr12530-fix-core-avatar-gro-0buuy2zc.web.app (expires Sun, 05 Jan 2025 18:03:28 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff |
<fd-avatar-group-default-example></fd-avatar-group-default-example> | ||
</component-example> | ||
<code-example [exampleFiles]="avatarGroupDefaultExample"></code-example> | ||
<!--<fd-docs-section-title id="default" componentName="AvatarGroup"> Avatar Group Standard Usage </fd-docs-section-title>--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not ready yet for review. I converted this PR as a draft. @mikerodonnell89
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but what is going on with all of the commented out html?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the "group type standard usage" examples, after opening the popover, i can not navigate through the avatars with the arrow keys or tab key. Because each avatar in the popover is clickable, we do need this functionality with keyboard control as well
@mikerodonnell89 In this example, the web components avatar group features tab navigation but lacks arrow navigation. I'm currently working on implementing tab navigation. |
@mikerodonnell89 Implemented arrow navigation. |
**/ | ||
avatars = input<AvatarGroupItemRendererDirective[]>([]); | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing declaration or wrong place for this jsDoc.
@mikerodonnell89 @dpavlenishvili Can you review it again? |
@dpavlenishvili any update? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not look like the first issue (regarding the focus staying on the first avatar group after interacting with a second) was fixed. In this example I've used the mouse to click both, then the escape key to close the second group
Screen.Recording.2024-11-25.at.2.45.53.PM.mov
@khotcholava Can you check the e2e? |
@@ -88,6 +94,10 @@ export class AvatarGroupComponent implements AvatarGroupHostConfig { | |||
@Input() | |||
size: AvatarGroupHostConfig['size'] = 'l'; | |||
|
|||
/** Popover placement */ | |||
@Input() | |||
popoverPlacement: Placement | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Nullable type.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@@ -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) => { |
There was a problem hiding this comment.
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();
}
});
`
…o fix(core)-avatar-group-bugs # Conflicts: # libs/core/popover/popover-body/popover-body.component.ts
@khotcholava needs to update with the comments from UX, check e2e/unit and make it as PR. Needs to be reviewed afterwards |
1 similar comment
@khotcholava needs to update with the comments from UX, check e2e/unit and make it as PR. Needs to be reviewed afterwards |
@khotcholava Can you summarize what you did and what needs to be implemented? |
fix(core): Resolve focus retention & rendering issues, add feature enhancements
Closes #12369
Description