AvatarStack: Add keyboard support to `AvatarStack` #5134

TylerJDev · 2024-10-16T16:17:11Z

Closes https://github.com/github/primer/issues/3348

Changelog

New

Adds keyboard support to AvatarStack

Changed

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-10-16T16:17:15Z

🦋 Changeset detected

Latest commit: 321553a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-10-16T16:58:06Z

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

github-actions · 2024-10-16T17:00:46Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	97.35 KB (+0.31% 🔺)
packages/react/dist/browser.umd.js	97.59 KB (+0.21% 🔺)

joshblack

Looking great! Left a couple comments/questions/suggestions 👀

joshblack · 2024-10-16T17:19:13Z

packages/react/src/internal/utils/getInteractiveNodes.ts

@@ -0,0 +1,23 @@
+const interactiveElements = [


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 👀

Definitely! Added some more selectors that could be handy.

joshblack · 2024-10-16T17:21:17Z

packages/react/src/internal/utils/getInteractiveNodes.ts

+  '[tabindex="0"]',
+]
+
+export function getInteractiveNodes(node: HTMLElement | null, ignoreSelectors?: string) {


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 {

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

joshblack · 2024-10-16T17:22:47Z

packages/react/src/AvatarStack/AvatarStack.tsx

@@ -195,6 +203,9 @@ const AvatarStack = ({
  className,
  sx: sxProp = defaultSxProp,
 }: AvatarStackProps) => {
+  const [hasInteractiveChildren, setHasInteractiveChildren] = useState<boolean | undefined>(false)
+  const AvatarStackContainer = useRef(null)


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)

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

joshblack · 2024-10-16T17:23:44Z

packages/react/src/AvatarStack/AvatarStack.tsx

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


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?

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

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();

};

}, []);

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

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.

primer-integration · 2024-10-16T17:26:38Z

🟢 golden-jobs completed with status success.

primer-integration · 2024-10-21T18:08:01Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/348018

TylerJDev · 2024-10-21T19:52:32Z

Requires small changes in Dotcom for tests, https://github.com/github/github/pull/348018/commits/833ad355c270f37218f3bcc4071319846b0e552a. Removes empty space that AvatarStack previously had. More context at https://github.com/github/memex/issues/16409.

joshblack · 2024-10-22T20:15:14Z

packages/react/src/AvatarStack/AvatarStack.tsx

+  useEffect(() => {
+    const hasInteractive = hasInteractiveNodes(avatarStackContainer.current, 'div[tabindex]')
+    setHasInteractiveChildren(hasInteractive)
+  }, [])


With the move to not running on children changing, wanted to ask if this is something that should be setup in a mutation observer for when contents change in case the "hasInteractiveNodes" check could change?

We could! I was thinking that we'd know whether to make the container interactive or not based on the initial render as I don't think there are cases where interactive items are rendered in after the first render. But we could add a mutation observer just to be safe.

Add keyboard support for AvatarStack

5a974b9

TylerJDev added 2 commits October 16, 2024 12:55

Add to tests, function

5dccb22

Merge branch 'main' into tylerjdev/avatarstack-keyboard-support

f26601b

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Oct 16, 2024

TylerJDev temporarily deployed to github-pages October 16, 2024 16:59 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 16, 2024 16:59 Inactive

Add changeset

dee9a1c

TylerJDev temporarily deployed to github-pages October 16, 2024 17:03 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 16, 2024 17:03 Inactive

Utilize focus styles for container

6090588

TylerJDev temporarily deployed to github-pages October 16, 2024 17:06 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 16, 2024 17:06 Inactive

TylerJDev requested a review from joshblack October 16, 2024 17:08

TylerJDev marked this pull request as ready for review October 16, 2024 17:08

TylerJDev requested a review from a team as a code owner October 16, 2024 17:08

joshblack reviewed Oct 16, 2024

View reviewed changes

Update getInteractiveNodes

8c1fc8d

TylerJDev temporarily deployed to github-pages October 17, 2024 13:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 17, 2024 13:44 Inactive

TylerJDev added 2 commits October 17, 2024 09:47

Change name

6d1f8e4

Change case

cb4fd73

TylerJDev temporarily deployed to github-pages October 17, 2024 13:50 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 17, 2024 13:50 Inactive

TylerJDev requested a review from joshblack October 17, 2024 14:03

Rework useEffect

a3417bb

TylerJDev temporarily deployed to github-pages October 21, 2024 16:04 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 21, 2024 16:04 Inactive

Merge branch 'main' into tylerjdev/avatarstack-keyboard-support

298262e

TylerJDev temporarily deployed to github-pages October 21, 2024 16:14 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 21, 2024 16:15 Inactive

Merge branch 'main' into tylerjdev/avatarstack-keyboard-support

f0df4af

TylerJDev temporarily deployed to github-pages October 21, 2024 18:03 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5134 October 21, 2024 18:04 Inactive

joshblack reviewed Oct 22, 2024

View reviewed changes

Add mutation observer, updates to hasInteractiveNodes

321553a

TylerJDev requested a review from joshblack October 23, 2024 12:58

TylerJDev deployed to github-pages October 23, 2024 12:59 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-5134 October 23, 2024 13:00 View deployment

joshblack approved these changes Oct 24, 2024

View reviewed changes

TylerJDev added this pull request to the merge queue Oct 25, 2024

Merged via the queue into main with commit 6713e72 Oct 25, 2024
43 checks passed

TylerJDev deleted the tylerjdev/avatarstack-keyboard-support branch October 25, 2024 13:23

primer bot mentioned this pull request Oct 25, 2024

Release tracking (37.1.0) #5166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AvatarStack: Add keyboard support to `AvatarStack` #5134

AvatarStack: Add keyboard support to `AvatarStack` #5134

TylerJDev commented Oct 16, 2024 •

edited

Loading

changeset-bot bot commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024

github-actions bot commented Oct 16, 2024 •

edited

Loading

joshblack left a comment

joshblack Oct 16, 2024

TylerJDev Oct 17, 2024

joshblack Oct 16, 2024

TylerJDev Oct 17, 2024

joshblack Oct 16, 2024

TylerJDev Oct 17, 2024

joshblack Oct 16, 2024

TylerJDev Oct 17, 2024

joshblack Oct 18, 2024

TylerJDev Oct 18, 2024

TylerJDev Oct 21, 2024

primer-integration bot commented Oct 16, 2024 •

edited

Loading

primer-integration bot commented Oct 21, 2024

TylerJDev commented Oct 21, 2024

joshblack Oct 22, 2024

TylerJDev Oct 23, 2024

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

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

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

AvatarStack: Add keyboard support to AvatarStack #5134

AvatarStack: Add keyboard support to AvatarStack #5134

Conversation

TylerJDev commented Oct 16, 2024 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Oct 16, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 16, 2024

github-actions bot commented Oct 16, 2024 • edited Loading

size-limit report 📦

joshblack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

primer-integration bot commented Oct 16, 2024 • edited Loading

primer-integration bot commented Oct 21, 2024

TylerJDev commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AvatarStack: Add keyboard support to `AvatarStack` #5134

AvatarStack: Add keyboard support to `AvatarStack` #5134

TylerJDev commented Oct 16, 2024 •

edited

Loading

changeset-bot bot commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

primer-integration bot commented Oct 16, 2024 •

edited

Loading