Add expand to NavList
#4686
Add expand to NavList
#4686
Conversation
🦋 Changeset detectedLatest commit: 88cbc80 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
I think going with parity is a good idea! I renamed it to |
| @@ -288,6 +290,54 @@ const Group: React.FC<NavListGroupProps> = ({title, children, sx: sxProp = defau | |||
| ) | |||
| } | |||
|
|
|||
| export type NavListShowMoreItemProps = { | |||
| children: React.ReactNode | |||
| label: string | |||
There was a problem hiding this comment.
Could this have a default value of "Show more" or is there really not a good default for the label that is used most of the time?
There was a problem hiding this comment.
We definitely could! I feel like most use cases will have context for what the items in the NavList are for. So adding a default "Show more" shouldn't hurt, as we allow consumers to customize it further if more context is needed.
| </ActionList.Item> | ||
| </Box> | ||
| ) : ( | ||
| <ItemWithinGroup.Provider value={{groupId}}>{children}</ItemWithinGroup.Provider> |
There was a problem hiding this comment.
Could we memoize this value since we're passing it to a context provider? Would help with unnecessary re-renders since this is an object defined inline.
|
|
||
| React.useEffect(() => { | ||
| if (expanded && !targetFocused.current) { | ||
| const focusTarget: HTMLAnchorElement | null = document.querySelector(`[data-show-more-group-id="${groupId}"]`) |
There was a problem hiding this comment.
Will this always be an HTMLAnchorElement or could it be another type? Couldn't quite tell from NavList if it forces <a> or not so wanted to double-check 👀
There was a problem hiding this comment.
I believe so. We don't seem to force it as NavList.Item as="button" works, but it's not officially supported (e.g. styles will most likely be incorrect if it's anything but a link <a>). I think it's a safe bet to rely on the HTMLAnchorElement, but we could make it more generic!
Co-authored-by: Josh Black <joshblack@github.com>
I don't think we do, but maybe this is something we'd want to have for future sub-components? 🤔 |
|
Pinging @primer/engineer-reviewers for any additional reviews! |
| if (expanded && !targetFocused.current) { | ||
| const focusTarget: HTMLAnchorElement | null = document.querySelector( | ||
| `[data-show-more-group-id="${groupId.id}"]`, | ||
| ) | ||
|
|
||
| if (focusTarget) { | ||
| focusTarget.focus() | ||
| targetFocused.current = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Wanted to ask if this could run in the onClick handler instead of us needing to synchronize this with an effect 👀 The benefit of tying this to the handler would be that focus would shift naturally due to interacting with the button instead of potentially being stolen if one the dependencies of the effect change that is not tied to a user interaction
There was a problem hiding this comment.
We could, but I think the only issue is the timing, as the node we want to focus once expanded doesn't exist yet 🤔
Something like the following works, but I'm not sure if it's counterintuitive or not 😅
onClick={() => {
flushSync(() => {
setExpanded(true)
})
expandItem()
}}
Adds new component
NavList.ShowMoreItem, allows native support for "expanding" content within aNavList.Closes https://github.com/github/primer/issues/2637
Proposed API
Basic example:
Multiple expands:
Group example (storybook)
Changelog
New
NavList.ShowMoreItemRollout strategy
Testing & Reviewing
Merge checklist