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

feat(avatar-groups): Add overflow #806

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Conversation

321gillian
Copy link
Collaborator

@321gillian 321gillian commented Dec 23, 2024

I think there's enough in this as it is so I've added another follow-on task for the next part: https://inindca.atlassian.net/browse/COMUI-3367

@321gillian 321gillian self-assigned this Dec 23, 2024
Copy link

@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch 2 times, most recently from 2b77284 to c291214 Compare December 23, 2024 14:32
visual-only
placement="top"
ref={el => (this.tooltip = el)}
<Host role="menuitem">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This role needs to be in the light dom, otherwise the menu won't announce the number of items in the menu

@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch from da7952d to 5b99fa4 Compare January 6, 2025 16:36
@321gillian 321gillian changed the title [WIP]feat(avatar-groups): Add overflow feat(avatar-groups): Add overflow Jan 6, 2025
@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch 2 times, most recently from fadc06d to 60dce21 Compare January 7, 2025 10:43
Comment on lines +116 to +115
resetFocusableSibling(clickedElement);
setFocusTarget(clickedElement);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just wondering what is the intended response when you click a gux-avatar-group-item-beta is the focus moved somewhere ?

Copy link
Collaborator Author

@321gillian 321gillian Jan 10, 2025

Choose a reason for hiding this comment

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

There's a convention in menus and avatar groups that I've seen where the last item visited by keyboard or clicked is set at the current focus target. So if you tab back up from below then you return to the last item clicked / the one you tabbed away from.

@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch 2 times, most recently from e5fc0cf to 46fce8c Compare January 10, 2025 17:10
@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch from 46fce8c to 3ccd099 Compare January 14, 2025 15:18
@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch from 3ccd099 to 0422877 Compare January 21, 2025 11:22
@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch from 0422877 to 840967e Compare January 22, 2025 17:40
@MyPureCloud MyPureCloud deleted a comment from 321gillian Jan 23, 2025
Copy link
Collaborator

@daragh-king-genesys daragh-king-genesys left a comment

Choose a reason for hiding this comment

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

LGTM

@Prop()
quantity: GuxAvatarGroupQuantity = Math.max(
...GROUP_QUANTITIES
) as GuxAvatarGroupQuantity;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we are purposely limiting the max quantity to the max of the number array? Is that why it's done this way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked this with Monse and we don't need to limit it to a max so I've updated it to remove the restriction.

@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch from 840967e to 0cdf1c1 Compare January 23, 2025 16:14
Copy link
Collaborator

@jordanstith15 jordanstith15 left a comment

Choose a reason for hiding this comment

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

LGTM

@321gillian 321gillian force-pushed the feature/COMUI-3050-overflow branch from 6709011 to 3f03442 Compare January 24, 2025 16:33
@321gillian 321gillian merged commit 8317321 into main Jan 24, 2025
3 checks passed
@321gillian 321gillian deleted the feature/COMUI-3050-overflow branch January 24, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants