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.

Sign up for GitHub

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

NavList: Add sx prop #2077

Merged
merged 4 commits into from
May 18, 2022
Merged

NavList: Add sx prop #2077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79b7fdc
Add support for `sx` prop to NavList components
colebemis May 16, 2022
105f3e5
Default sx to empty object
colebemis May 16, 2022
34e7e6b
Create happy-brooms-swim.md
colebemis May 16, 2022
3ff0d7c
Update snapshot
colebemis May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/happy-brooms-swim.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Adds support for the `sx` prop on the draft implementation of `NavList` and all its subcomponents (e.g., `NavList.Item`)
75 changes: 47 additions & 28 deletions src/NavList/NavList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,27 @@
import {ChevronDownIcon} from '@primer/octicons-react'
import {useSSRSafeId} from '@react-aria/ssr'
import React, {isValidElement} from 'react'
import styled from 'styled-components'
import {ActionList} from '../ActionList'
import Box from '../Box'
import StyledOcticon from '../StyledOcticon'
import sx, {merge, SxProp} from '../sx'

// ----------------------------------------------------------------------------
// NavList

export type NavListProps = {
children: React.ReactNode
} & React.ComponentProps<'nav'>
} & SxProp &
React.ComponentProps<'nav'>

const NavBox = styled.nav<SxProp>(sx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this go against ADR 005? https://github.com/primer/react/blob/main/contributor-docs/adrs/adr-005-box-sx.md

cc @siddharthkp

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 copied this from ActionList (code) as a workaround for a ref type error I ran into.

// TypeScript doesn't like this
<Box as="nav" ref={ref} sx={{...}}>

// But this works
const NavBox = styled.nav(sx)
<NavBox ref={ref} sx={{...}}>

If there's another way to workaround that type issue, I'm open to updating this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I've also struggled with this type issue, and I'm not sure how to fix it 😅

Let's leave this overnight and see if any of our coworkers in other timezones know a solution.

Copy link
Member

@siddharthkp siddharthkp May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I remember this now :( I tried a bunch of things, but couldn't come up with something better.

For Avatar, I was able to come up with something better because it doesn't need a polymorphic ref, it's always img. Not sure if that's relevant for NavList

We need an ADR for types and polymorphic refs, I don't feel confident about a recommended approach to write that one though 😓

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'm going to move forward with this for now. A long-term solution might be to move away from the as prop entirely 😅


// TODO: sx prop
const Root = React.forwardRef<HTMLElement, NavListProps>(({children, ...props}, ref) => {
return (
<nav ref={ref} {...props}>
<NavBox {...props} ref={ref}>
<ActionList>{children}</ActionList>
</nav>
</NavBox>
)
})

Expand All @@ -30,12 +34,11 @@ export type NavListItemProps = {
children: React.ReactNode
href?: string
'aria-current'?: 'page' | 'step' | 'location' | 'date' | 'time' | 'true' | 'false' | boolean
}
} & SxProp

// TODO: sx prop
// TODO: as prop
const Item = React.forwardRef<HTMLAnchorElement, NavListItemProps>(
({href, 'aria-current': ariaCurrent, children}, ref) => {
({href, 'aria-current': ariaCurrent, children, sx: sxProp = {}}, ref) => {
const {depth} = React.useContext(SubNavContext)

// Get SubNav from children
Expand All @@ -54,7 +57,7 @@ const Item = React.forwardRef<HTMLAnchorElement, NavListItemProps>(
)

return (
<ItemWithSubNav subNav={subNav} subNavContainsCurrentItem={Boolean(currentItem)}>
<ItemWithSubNav subNav={subNav} subNavContainsCurrentItem={Boolean(currentItem)} sx={sxProp}>
{childrenWithoutSubNav}
</ItemWithSubNav>
)
Expand All @@ -66,11 +69,14 @@ const Item = React.forwardRef<HTMLAnchorElement, NavListItemProps>(
href={href}
aria-current={ariaCurrent}
active={Boolean(ariaCurrent) && ariaCurrent !== 'false'}
sx={{
paddingLeft: depth > 0 ? 5 : null, // Indent sub-items
fontSize: depth > 0 ? 0 : null, // Reduce font size of sub-items
fontWeight: depth > 0 ? 'normal' : null // Sub-items don't get bolded
}}
sx={merge<SxProp['sx']>(
{
paddingLeft: depth > 0 ? 5 : null, // Indent sub-items
fontSize: depth > 0 ? 0 : null, // Reduce font size of sub-items
fontWeight: depth > 0 ? 'normal' : null // Sub-items don't get bolded
},
sxProp
)}
>
{children}
</ActionList.LinkItem>
Expand All @@ -87,17 +93,16 @@ type ItemWithSubNavProps = {
children: React.ReactNode
subNav: React.ReactNode
subNavContainsCurrentItem: boolean
}
} & SxProp

const ItemWithSubNavContext = React.createContext<{buttonId: string; subNavId: string}>({
buttonId: '',
subNavId: ''
})

// TODO: sx prop
// TODO: ref prop
// TODO: Animate open/close transition
function ItemWithSubNav({children, subNav, subNavContainsCurrentItem}: ItemWithSubNavProps) {
function ItemWithSubNav({children, subNav, subNavContainsCurrentItem, sx: sxProp = {}}: ItemWithSubNavProps) {
const buttonId = useSSRSafeId()
const subNavId = useSSRSafeId()
// SubNav starts open if current item is in it
Expand All @@ -114,9 +119,12 @@ function ItemWithSubNav({children, subNav, subNavContainsCurrentItem}: ItemWithS
// When the subNav is closed, how should we indicated that the subNav contains the current item?
active={!isOpen && subNavContainsCurrentItem}
onClick={() => setIsOpen(open => !open)}
sx={{
fontWeight: subNavContainsCurrentItem ? 'bold' : null // Parent item is bold if any of it's sub-items are current
}}
sx={merge<SxProp['sx']>(
{
fontWeight: subNavContainsCurrentItem ? 'bold' : null // Parent item is bold if any of it's sub-items are current
},
sxProp
)}
>
{children}
{/* What happens if the user provides a TrailingVisual? */}
Expand All @@ -141,14 +149,13 @@ function ItemWithSubNav({children, subNav, subNavContainsCurrentItem}: ItemWithS

type NavListSubNavProps = {
children: React.ReactNode
}
} & SxProp

const SubNavContext = React.createContext<{depth: number}>({depth: 0})

// TODO: sx prop
// TODO: ref prop
// NOTE: SubNav must be a direct child of an Item
const SubNav = ({children}: NavListSubNavProps) => {
const SubNav = ({children, sx: sxProp = {}}: NavListSubNavProps) => {
const {buttonId, subNavId} = React.useContext(ItemWithSubNavContext)
const {depth} = React.useContext(SubNavContext)

Expand All @@ -165,7 +172,18 @@ const SubNav = ({children}: NavListSubNavProps) => {

return (
<SubNavContext.Provider value={{depth: depth + 1}}>
<Box as="ul" id={subNavId} aria-labelledby={buttonId} sx={{padding: 0, margin: 0}}>
<Box
as="ul"
id={subNavId}
aria-labelledby={buttonId}
sx={merge<SxProp['sx']>(
{
padding: 0,
margin: 0
},
sxProp
)}
>
{children}
</Box>
</SubNavContext.Provider>
Expand Down Expand Up @@ -198,19 +216,20 @@ Divider.displayName = 'NavList.Divider'
// ----------------------------------------------------------------------------
// NavList.Group

type NavListGroupProps = React.PropsWithChildren<{
type NavListGroupProps = {
children: React.ReactNode
title?: string
}>
} & SxProp

// TODO: sx prop
// TODO: ref prop
const Group = ({title, children}: NavListGroupProps) => {
const Group = ({title, children, sx: sxProp = {}}: NavListGroupProps) => {
return (
<>
{/* Hide divider if the group is the first item in the list */}
<ActionList.Divider sx={{'&:first-child': {display: 'none'}}} />
<ActionList.Group title={title}>{children}</ActionList.Group>
<ActionList.Group title={title} sx={sxProp}>
{children}
</ActionList.Group>
</>
)
}
Expand Down
16 changes: 12 additions & 4 deletions src/NavList/__snapshots__/NavList.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,9 @@ exports[`NavList renders a simple list 1`] = `
}

<div>
<nav>
<nav
class=""
>
<ul
class="c0"
>
Expand Down Expand Up @@ -695,7 +697,9 @@ exports[`NavList renders with groups 1`] = `
}

<div>
<nav>
<nav
class=""
>
<ul
class="c0"
>
Expand Down Expand Up @@ -1172,7 +1176,9 @@ exports[`NavList.Item with NavList.SubNav does not have active styles if SubNav
}

<div>
<nav>
<nav
class=""
>
<ul
class="c0"
>
Expand Down Expand Up @@ -1480,7 +1486,9 @@ exports[`NavList.Item with NavList.SubNav has active styles if SubNav contains t
}

<div>
<nav>
<nav
class=""
>
<ul
class="c0"
>
Expand Down