feat: S2 SideNav#10306
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| // RAC swallows arrow keys at the collection level (stopPropagation during capture), so a handler | ||
| // on the link never sees them. Intercept here on an ancestor, before RAC's row handler runs, and | ||
| // expand a collapsed row when the expand arrow is pressed while focus is on its link. | ||
| let onKeyDownCapture = (e: ReactKeyboardEvent) => { |
There was a problem hiding this comment.
I haven't pulled this down to try it myself, but perhaps you could make SideNav a keyboardNavigationBehavior = tab tree and have focusMode="child" and allowsArrowNavigation=true so that arrow up/down navigation still work but focus lands on the link by default. useGridList's handleTreeExpansionKeys will have to be updated to also check allowsArrowNavigation so that it can skip the
activeElement check below, but that might allow you to get rid of all this extra handling at this level
if (!('expandedKeys' in state) || activeElement !== rowRef) {
return false;
}
note that the extra props mentioned above are in #10159, so will require you to rebase/pull in that PR
There was a problem hiding this comment.
Good news, textfield_gridlists is already my base :)
I attempted an implementation of the above and it appears to mostly work, so that's pretty awesome. I was missing that allowsArrowNavigation update when I tried those all together previously. Good thinking.
Just for posterity, there's a couple cases here:
Left/Right arrow to expand/collapse/traverse up that should still be accessible from the link.
In addition, focusMode="child" should only be the case for items with links (not categories) because then it might accidentally focus something like an ActionMenu that may be in the row regardless of if there is a Link.
Shift+Tab from a link should leave the SideNav.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
There was a problem hiding this comment.
Looks good to me, verified the recent fixes, just some small comments
| import {useLocale} from 'react-aria/I18nProvider'; | ||
| import {useScale} from './utils'; | ||
|
|
||
| export interface SideNavProps<T> |
There was a problem hiding this comment.
Should SideNav have onAction exposed on it? I thought routing would be triggered via the SideNav links
| TreeStateContext | ||
| } from '../src/Tree'; | ||
| export type { | ||
| GridListSectionProps, |
There was a problem hiding this comment.
Should this be redefined as TreeSectionProps so if they diverge in the future it will be easier to handle?
|
Build successful! 🎉 |
| forcedColors: 'ButtonText' | ||
| }, | ||
| fontWeight: { | ||
| isSelected: 'bold', |
There was a problem hiding this comment.
Not ideal that when you select an item the entire sidenav gets wider and causes a layout shift. You can see this in the docs example when you select "Background layers"
There was a problem hiding this comment.
hmmm that is a good point. I could render a visually hidden zero height block element to reserve the horizontal space... I'll push that up and you can see how you like it
There was a problem hiding this comment.
think I should hardcode the width as well though? when you collapse a level it also causes a shift
| SideNavs do not support an uncontrolled selection state, you are responsible for managing it through the `selectedRoute` prop. You may wire this up | ||
| to a router or other state management solution. | ||
|
|
||
| If a SideNavItem has an `href`, then you must pass a `SideNavItemLink` as a child of the `SideNavItemContent`. |
There was a problem hiding this comment.
I don't think we need these caveats. Instead of saying how it's different, just describe how it works. We don't need to frame it as a collection component in the docs. An example highlighting the selectedRoute prop would also be helpful. Maybe rename the section "Routing" (also move it after Content since that is usually first) and use it to describe how to hook up RouterProvider?
| >, | ||
| UnsafeStyles { | ||
| /** The route that is currently selected. */ | ||
| selectedRoute: string; |
There was a problem hiding this comment.
What do you think about having this automatically set by the RouterProvider context?
There was a problem hiding this comment.
I wanted selectedRoute to be a required prop in case someone wanted to use something other than the RouterProvider. Do you think that's reasonable?
| inset: 0, | ||
| top: 0, | ||
| bottom: 0, | ||
| borderRadius: 'default', // tokens say 12... but that seems a lot, should it match selection in other collections? |
There was a problem hiding this comment.
feel like it makes sense to just match the other collections which reminds me that we need to update TreeView's to match ListView still...
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:GridListHeaderProps+GridListHeaderProps {
+ children?: ReactNode
+ className?: string
+ id?: string
+ render?: DOMRenderFunction<keyof React.JSX.IntrinsicElements, undefined>
+ style?: CSSProperties
+}@react-aria/calendar/@react-aria/calendar:useCalendarMonthPicker-useCalendarMonthPicker {
- props: CalendarMonthPickerProps
- state: CalendarState<CalendarSelectionMode> | RangeCalendarState
- returnVal: undefined
-}/@react-aria/calendar:useCalendarYearPicker-useCalendarYearPicker {
- props: CalendarYearPickerProps
- state: CalendarState<CalendarSelectionMode> | RangeCalendarState
- returnVal: undefined
-}/@react-aria/calendar:CalendarMonthPickerAria-CalendarMonthPickerAria {
- aria-label: string
- items: Array<CalendarMonthPickerItem>
- onChange: (Key | null) => void
- value: Key
-}/@react-aria/calendar:CalendarMonthPickerItem-CalendarMonthPickerItem {
- date: CalendarDate
- formatted: string
- id: number
-}/@react-aria/calendar:CalendarMonthPickerProps-CalendarMonthPickerProps {
- format?: 'numeric' | '2-digit' | 'long' | 'short' | 'narrow'
-}/@react-aria/calendar:CalendarYearPickerAria-CalendarYearPickerAria {
- aria-label: string
- items: Array<CalendarYearPickerItem>
- onChange: (Key | null) => void
- value: Key
-}/@react-aria/calendar:CalendarYearPickerItem-CalendarYearPickerItem {
- date: CalendarDate
- formatted: string
- id: number
-}/@react-aria/calendar:CalendarYearPickerProps-CalendarYearPickerProps {
- format?: CalendarYearPickerFormatOptions
- visibleYears?: number = 20
-}@react-spectrum/s2/@react-spectrum/s2:SideNav+SideNav <T> {
+ UNSAFE_className?: UnsafeClassName
+ UNSAFE_style?: CSSProperties
+ aria-describedby?: string
+ aria-details?: string
+ aria-label?: string
+ aria-labelledby?: string
+ autoFocus?: boolean | FocusStrategy
+ children?: ReactNode | (T) => ReactNode
+ defaultExpandedKeys?: Iterable<Key>
+ dependencies?: ReadonlyArray<any>
+ disabledKeys?: Iterable<Key>
+ expandedKeys?: Iterable<Key>
+ id?: string
+ items?: Iterable<T>
+ onExpandedChange?: (Set<Key>) => any
+ selectedRoute: string
+ slot?: string | null
+ styles?: StylesPropWithHeight
+}/@react-spectrum/s2:SideNavItem+SideNavItem {
+ aria-label?: string
+ children: ReactNode
+ download?: boolean | string
+ hasChildItems?: boolean
+ href?: Href
+ hrefLang?: string
+ id?: Key
+ isDisabled?: boolean
+ onHoverChange?: (boolean) => void
+ onHoverEnd?: (HoverEvent) => void
+ onHoverStart?: (HoverEvent) => void
+ onPress?: (PressEvent) => void
+ onPressChange?: (boolean) => void
+ onPressEnd?: (PressEvent) => void
+ onPressStart?: (PressEvent) => void
+ onPressUp?: (PressEvent) => void
+ ping?: string
+ referrerPolicy?: HTMLAttributeReferrerPolicy
+ rel?: string
+ routerOptions?: RouterOptions
+ target?: HTMLAttributeAnchorTarget
+ textValue: string
+}/@react-spectrum/s2:SideNavItemContent+SideNavItemContent {
+ children: ReactNode
+}/@react-spectrum/s2:SideNavItemLink+SideNavItemLink {
+ children?: ReactNode
+}/@react-spectrum/s2:SideNavSection+SideNavSection <T extends {}> {
+ aria-label?: string
+ children?: ReactNode | (T) => ReactElement
+ dependencies?: ReadonlyArray<any>
+ id?: Key
+ items?: Iterable<T>
+}/@react-spectrum/s2:SideNavHeader+SideNavHeader {
+ children?: ReactNode
+ id?: string
+}/@react-spectrum/s2:SideNavProps+SideNavProps <T> {
+ UNSAFE_className?: UnsafeClassName
+ UNSAFE_style?: CSSProperties
+ aria-describedby?: string
+ aria-details?: string
+ aria-label?: string
+ aria-labelledby?: string
+ autoFocus?: boolean | FocusStrategy
+ children?: ReactNode | (T) => ReactNode
+ defaultExpandedKeys?: Iterable<Key>
+ dependencies?: ReadonlyArray<any>
+ disabledKeys?: Iterable<Key>
+ expandedKeys?: Iterable<Key>
+ id?: string
+ items?: Iterable<T>
+ onExpandedChange?: (Set<Key>) => any
+ selectedRoute: string
+ slot?: string | null
+ styles?: StylesPropWithHeight
+}/@react-spectrum/s2:SideNavItemProps+SideNavItemProps {
+ aria-label?: string
+ children: ReactNode
+ download?: boolean | string
+ hasChildItems?: boolean
+ href?: Href
+ hrefLang?: string
+ id?: Key
+ isDisabled?: boolean
+ onHoverChange?: (boolean) => void
+ onHoverEnd?: (HoverEvent) => void
+ onHoverStart?: (HoverEvent) => void
+ onPress?: (PressEvent) => void
+ onPressChange?: (boolean) => void
+ onPressEnd?: (PressEvent) => void
+ onPressStart?: (PressEvent) => void
+ onPressUp?: (PressEvent) => void
+ ping?: string
+ referrerPolicy?: HTMLAttributeReferrerPolicy
+ rel?: string
+ routerOptions?: RouterOptions
+ target?: HTMLAttributeAnchorTarget
+ textValue: string
+}/@react-spectrum/s2:SideNavItemContentProps+SideNavItemContentProps {
+ children: ReactNode
+}/@react-spectrum/s2:SideNavItemLinkProps+SideNavItemLinkProps {
+ children?: ReactNode
+}/@react-spectrum/s2:SideNavSectionProps+SideNavSectionProps <T> {
+ aria-label?: string
+ children?: ReactNode | (T) => ReactElement
+ dependencies?: ReadonlyArray<any>
+ id?: Key
+ items?: Iterable<T>
+}/@react-spectrum/s2:SideNavHeaderProps+SideNavHeaderProps {
+ children?: ReactNode
+ id?: string
+} |
Closes
Early draft of SideNav for S2.
api is this right now:
decisions to make still
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: