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

AvatarStack: Add keyboard support to AvatarStack #5134

Merged
merged 12 commits into from
Oct 25, 2024
5 changes: 5 additions & 0 deletions .changeset/shy-seahorses-mix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

AvatarStack: Adds keyboard support to `AvatarStack`
28 changes: 23 additions & 5 deletions packages/react/src/AvatarStack/AvatarStack.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {clsx} from 'clsx'
import React from 'react'
import React, {useEffect, useRef, useState} from 'react'
import styled from 'styled-components'
import {get} from '../constants'
import Box from '../Box'
Expand All @@ -12,6 +12,8 @@ import {isResponsiveValue} from '../hooks/useResponsiveValue'
import {getBreakpointDeclarations} from '../utils/getBreakpointDeclarations'
import {defaultSxProp} from '../utils/defaultSxProp'
import type {WidthOnlyViewportRangeKeys} from '../utils/types/ViewportRangeKeys'
import {getInteractiveNodes} from '../internal/utils/getInteractiveNodes'
import getGlobalFocusStyles from '../internal/utils/getGlobalFocusStyles'

type StyledAvatarStackWrapperProps = {
count?: number
Expand All @@ -30,6 +32,8 @@ const AvatarStackWrapper = styled.span<StyledAvatarStackWrapperProps>`
.pc-AvatarStackBody {
display: flex;
position: absolute;

${getGlobalFocusStyles('1px')}
}

.pc-AvatarItem {
Expand Down Expand Up @@ -130,7 +134,8 @@ const AvatarStackWrapper = styled.span<StyledAvatarStackWrapperProps>`
.pc-AvatarStackBody {
flex-direction: row-reverse;

&:not(.pc-AvatarStack--disableExpand):hover {
&:not(.pc-AvatarStack--disableExpand):hover,
&:not(.pc-AvatarStack--disableExpand):focus-within {
.pc-AvatarItem {
margin-right: ${get('space.1')}!important;
margin-left: 0 !important;
Expand All @@ -143,7 +148,8 @@ const AvatarStackWrapper = styled.span<StyledAvatarStackWrapperProps>`
}
}

.pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover {
.pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover,
.pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within {
width: auto;

.pc-AvatarItem {
Expand All @@ -157,6 +163,8 @@ const AvatarStackWrapper = styled.span<StyledAvatarStackWrapperProps>`
visibility 0.2s ease-in-out,
box-shadow 0.1s ease-in-out;

${getGlobalFocusStyles('1px')}

&:first-child {
margin-left: 0;
}
Expand Down Expand Up @@ -195,6 +203,9 @@ const AvatarStack = ({
className,
sx: sxProp = defaultSxProp,
}: AvatarStackProps) => {
const [hasInteractiveChildren, setHasInteractiveChildren] = useState<boolean | undefined>(false)
const AvatarStackContainer = useRef(null)
Copy link
Member

Choose a reason for hiding this comment

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

style nit (and definitely non blocking lol) we could use lowercase for useRef here since we typically only use pascal case for component names or modules

Suggested change
const AvatarStackContainer = useRef(null)
const avatarStackContainer = useRef(null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize I was using pascal case 🤦 Should be updated now!


const count = React.Children.count(children)
const wrapperClassNames = clsx(
{
Expand Down Expand Up @@ -249,6 +260,11 @@ const AvatarStack = ({
)
}

useEffect(() => {
const hasInteractive = getInteractiveNodes(AvatarStackContainer.current, '[tabindex]')
setHasInteractiveChildren(hasInteractive)
}, [children])
Copy link
Member

Choose a reason for hiding this comment

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

Would this be equivalent to:

useEffect(() => {
  // ...
})

I wasn't sure if children would change each render or not if they are not memoized. If that's the case, would it be worth running as a mutation observer, potentially?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could! My assumption is that we'd want to utilize the mutation observer inside of hasInteractiveNodes?

Copy link
Member

Choose a reason for hiding this comment

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

I think you could use it in there if the function was updated to have a callback/subscriber for when it changed or could use the mutation observer directly and call hasInteractiveNodes in it, if that makes sense. Put another way:

Suggested change
}, [children])
useEffect(() => {
const observer = new MutationObserver((entries) => {
setHasInteractiveChildren(hasInteractiveContent(ref.current));
});
observer.observer(ref.current, {
/* whatever options make sense */
});
return () => {
observer.disconnect();
};
}, []);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, this is great! Thank you for the example too. I'll rework this a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After implementing this, I realize that the need for checking the children for changes is probably very slim 😅 I removed the children dependency so it should only run on the initial render, as that's the only time we'd actually want to run this function/check.


const getResponsiveAvatarSizeStyles = () => {
// if there is no size set on the AvatarStack, use the `size` props of the Avatar children to set the `--avatar-stack-size` CSS variable
if (!size) {
Expand Down Expand Up @@ -278,8 +294,10 @@ const AvatarStack = ({
)

return (
<AvatarStackWrapper count={count} className={wrapperClassNames} sx={avatarStackSx}>
<Box className={bodyClassNames}> {transformChildren(children)}</Box>
<AvatarStackWrapper count={count} className={wrapperClassNames} sx={avatarStackSx} ref={AvatarStackContainer}>
<Box className={bodyClassNames} tabIndex={!hasInteractiveChildren && !disableExpand ? 0 : undefined}>
{transformChildren(children)}
</Box>
</AvatarStackWrapper>
)
}
Expand Down
24 changes: 24 additions & 0 deletions packages/react/src/__tests__/AvatarStack.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,28 @@ describe('Avatar', () => {
it('respects alignRight props', () => {
expect(render(rightAvatarComp)).toMatchSnapshot()
})

it('should have a tabindex of 0 if there are no interactive children', () => {
const {container} = HTMLRender(avatarComp)
expect(container.querySelector('[tabindex="0"]')).toBeInTheDocument()
})

it('should not have a tabindex if there are interactive children', () => {
const {container} = HTMLRender(
<AvatarStack>
<button type="button">Click me</button>
</AvatarStack>,
)
expect(container.querySelector('[tabindex="0"]')).not.toBeInTheDocument()
})

it('should not have a tabindex if disableExpand is true', () => {
const {container} = HTMLRender(
<AvatarStack disableExpand>
<img src="https://avatars.githubusercontent.com/primer" alt="" />
<img src="https://avatars.githubusercontent.com/github" alt="" />
</AvatarStack>,
)
expect(container.querySelector('[tabindex="0"]')).not.toBeInTheDocument()
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -107,32 +107,57 @@ exports[`Avatar respects alignRight props 1`] = `
flex-direction: row-reverse;
}

.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem {
.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem,
.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem {
margin-right: 4px!important;
margin-left: 0 !important;
}

.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child {
.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child,
.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:first-child {
margin-right: 0 !important;
}

.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover {
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover,
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within {
width: auto;
}

.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem {
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem,
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem {
margin-left: 4px;
opacity: 100%;
visibility: visible;
-webkit-transition: margin 0.2s ease-in-out,opacity 0.2s ease-in-out,visibility 0.2s ease-in-out,box-shadow 0.1s ease-in-out;
transition: margin 0.2s ease-in-out,opacity 0.2s ease-in-out,visibility 0.2s ease-in-out,box-shadow 0.1s ease-in-out;
}

.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem box-shadow:inset 0 0 0 4px function (props) {
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem box-shadow:inset 0 0 0 4px function (props),
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem box-shadow:inset 0 0 0 4px function (props) {
return: (0,_core.get)(props.theme,path,fallback);
}

.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child {
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:focus:not(:disabled),
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:focus:not(:disabled) {
box-shadow: none;
outline: 2px solid var(--fgColor-accent,var(--color-accent-fg,#0969da));
outline-offset: 1px;
}

.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:focus:not(:disabled):not(:focus-visible),
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:focus:not(:disabled):not(:focus-visible) {
outline: solid 1px transparent;
}

.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:focus-visible:not(:disabled),
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:focus-visible:not(:disabled) {
box-shadow: none;
outline: 2px solid var(--fgColor-accent,var(--color-accent-fg,#0969da));
outline-offset: 1px;
}

.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child,
.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:first-child {
margin-left: 0;
}

Expand All @@ -145,8 +170,8 @@ exports[`Avatar respects alignRight props 1`] = `
>
<div
className="pc-AvatarStackBody"
tabIndex={0}
>

<img
alt=""
className="pc-AvatarItem"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import {getInteractiveNodes} from '../getInteractiveNodes'

describe('getInteractiveNodes', () => {
test('if there are no interactive nodes', () => {
const node = document.createElement('div')
expect(getInteractiveNodes(node)).toBeUndefined()
})

test('if there are interactive nodes', () => {
const node = document.createElement('div')
const button = document.createElement('button')
node.appendChild(button)

expect(getInteractiveNodes(node)).toBe(true)
})

test('if the node itself is interactive', () => {
const node = document.createElement('button')

expect(getInteractiveNodes(node)).toBe(true)
})

test('if there are nested interactive nodes', () => {
const node = document.createElement('div')
const wrapper = document.createElement('div')
const button = document.createElement('button')
const span = document.createElement('span')
wrapper.appendChild(button)
button.appendChild(span)
node.appendChild(wrapper)

expect(getInteractiveNodes(node)).toBe(true)
})

test('if the node is disabled', () => {
const node = document.createElement('button')
node.disabled = true

expect(getInteractiveNodes(node)).toBeUndefined()
})

test('if the child node is disabled', () => {
const node = document.createElement('div')
const button = document.createElement('button')
button.disabled = true
node.appendChild(button)

expect(getInteractiveNodes(node)).toBeUndefined()
})

test('if child node has tabindex', () => {
const node = document.createElement('div')
const span = document.createElement('span')
span.setAttribute('tabindex', '0')
node.appendChild(span)

expect(getInteractiveNodes(node)).toBe(true)
})
})
23 changes: 23 additions & 0 deletions packages/react/src/internal/utils/getInteractiveNodes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const interactiveElements = [
Copy link
Member

Choose a reason for hiding this comment

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

One package we could use for this: https://www.npmjs.com/package/focusable-selectors (either as a dependency or inline with accreditation) that has a pretty robust list 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! Added some more selectors that could be handy.

'a[href]',
'button:not([disabled])',
'summary',
'select',
'input:not([type=hidden])',
'textarea',
'[tabindex="0"]',
]

export function getInteractiveNodes(node: HTMLElement | null, ignoreSelectors?: string) {
Copy link
Member

Choose a reason for hiding this comment

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

One style suggestion, since we're returning a boolean this could be hasInteractiveNodes as opposed to getInteractiveNodes which might seem like we are returning the list of interactive nodes within the given parent node.

Suggested change
export function getInteractiveNodes(node: HTMLElement | null, ignoreSelectors?: string) {
export function hasInteractiveContent(node: HTMLElement | null, ignoreSelectors?: string): boolean {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good callout. I changed the names of the function + file!

if (!node) return

let interactiveNodes = Array.from(node.querySelectorAll(interactiveElements.join(', ')))

if (ignoreSelectors) {
interactiveNodes = interactiveNodes.filter(node => !node.matches(ignoreSelectors))
}

if (interactiveNodes.length || node.matches(interactiveElements.join(', '))) {
return true
}
}
Loading