-
Notifications
You must be signed in to change notification settings - Fork 1.4k
chore: update disabledBehavior style for S2 TreeView
#9472
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
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| [ActionButtonGroupContext, {styles: treeActions, isDisabled}], | ||
| [ActionMenuContext, {styles: treeActionMenu, isQuiet: true, isDisabled}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think these should only be disabled if the disabledBehaviour is 'all', not just if the item is disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isDisabled here comes from TreeState which determines isDisabled from a variety of different things. isDisabled will be false if disabledBehavior is selection
react-spectrum/packages/@react-stately/selection/src/SelectionManager.ts
Lines 504 to 506 in 76f539b
| isDisabled(key: Key): boolean { | |
| return this.state.disabledBehavior === 'all' && (this.state.disabledKeys.has(key) || !!this.collection.getItem(key)?.props?.isDisabled); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, that was really hidden up there, thanks.
can you write a test for this? seems easy to accidentally regress on this one
| }); | ||
| }, | ||
| renderers: { | ||
| // todo - we don't support isDisabled on TreeViewItems? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a comment left by Rob when TreeView was first implemented in S2...not sure if this is relevant anymore? we support isDisabled on TreeViewItems according to the docs
|
Build successful! 🎉 |
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:CellRenderProps CellRenderProps {
- colIndex?: number | null
id?: Key
isFocusVisible: boolean
isFocused: boolean
isHovered: boolean
isSelected: boolean
}@react-aria/interactions/@react-aria/interactions:useFocusVisibleListener useFocusVisibleListener {
fn: FocusVisibleHandler
deps: ReadonlyArray<any>
opts?: {
- enabled?: boolean
- isTextInput?: boolean
+ isTextInput?: boolean
}
returnVal: undefined
}@react-spectrum/s2/@react-spectrum/s2:MenuSection MenuSection <T extends {}> {
aria-label?: string
- children?: ReactNode | (T) => ReactElement
+ children?: ReactNode
defaultSelectedKeys?: 'all' | Iterable<Key>
dependencies?: ReadonlyArray<any>
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
items?: Iterable<T>
onSelectionChange?: (Selection) => void
selectedKeys?: 'all' | Iterable<Key>
selectionMode?: SelectionMode
shouldCloseOnSelect?: boolean
value?: T
}/@react-spectrum/s2:MenuSectionProps MenuSectionProps <T extends {}> {
aria-label?: string
- children?: ReactNode | (T) => ReactElement
+ children?: ReactNode
defaultSelectedKeys?: 'all' | Iterable<Key>
dependencies?: ReadonlyArray<any>
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
items?: Iterable<T>
onSelectionChange?: (Selection) => void
selectedKeys?: 'all' | Iterable<Key>
selectionMode?: SelectionMode
shouldCloseOnSelect?: boolean
value?: T
} |
| TreeViewItemContent | ||
| } from '../src'; | ||
|
|
||
| AriaTreeTests({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we deleting this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, i see it's a rename
| let menu = queryByRole('menu'); | ||
| expect(menu).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is menu === null here?
not.toBeInTheDocument is a bit of a weird assertion for that, but i guess it works 🤷🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ig i could change it to expect(menu).toBeNull()?
chromatic: https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=1101
make sure it matches the new proposed changes: RSP Component Milestones (view)
✅ Pull Request Checklist:
📝 Test Instructions:
Go to S2 storybook -> TreeView -> mess around with the disabledBehavior control
🧢 Your Project: