-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(table): S2 tableview custom column menu #7617
base: main
Are you sure you want to change the base?
Conversation
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.
It looks like this broke alignment in the resizing table header columns.
https://reactspectrum.blob.core.windows.net/reactspectrum/e6c04fecb3a59548fc621b334b4c75d3a2879cf4/storybook-s2/index.html?path=/story/tableview--resizing-table
https://reactspectrum.blob.core.windows.net/reactspectrum/e6c04fecb3a59548fc621b334b4c75d3a2879cf4/storybook-s2/index.html?path=/story/tableview--resizing-sortable-table
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.
LGTM
{(item) => <MenuItem>{item?.label}</MenuItem>} | ||
<Menu onAction={onMenuSelect} styles={style({minWidth: 128})}> | ||
{items.length > 0 && ( | ||
<MenuSection aria-label='Sort or resize?'> |
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.
what's a label that actually makes sense here? multiple labels depending on if it's one or the other or both?
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.
Shouldn't this be a string key for i18n?
What about something nonspecific "Column options"? There will only be actions we take on a column, right?
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.
yes, i'm trying to figure out what it should read first though, or if we even want to have this be a section
<MenuSection aria-label='Sort or resize?'> | ||
<Collection items={items}> | ||
{(item) => <MenuItem>{item?.label}</MenuItem>} | ||
</Collection> |
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.
best way to add other sort options here (for other columns like doc cloud has?)
## API Changes
@react-aria/focus/@react-aria/focus:isFocusable isFocusable {
- element: Element
+ element: HTMLElement
returnVal: undefined
} @react-aria/test-utils/@react-aria/test-utils:triggerLongPress triggerLongPress {
opts: {
element: HTMLElement
advanceTimer: (number) => void | Promise<unknown>
- pointerOpts?: Record<string, any>
+ pointerOpts?: {
+
}
+}
returnVal: undefined
} @react-aria/utils/@react-aria/utils:isFocusable-isFocusable {
- element: Element
- returnVal: undefined
-} /@react-aria/utils:isTabbable-isTabbable {
- element: Element
- returnVal: undefined
-} @react-spectrum/s2/@react-spectrum/s2:Column Column {
align?: 'start' | 'center' | 'end' = 'start'
allowsResizing?: boolean
allowsSorting?: boolean
children: ReactNode
className?: string | ((ColumnRenderProps & {
defaultClassName: string | undefined
})) => string
defaultWidth?: ColumnSize | null
id?: Key
isRowHeader?: boolean
maxWidth?: ColumnStaticSize | null
+ menu?: ReactNode
minWidth?: ColumnStaticSize | null
showDivider?: boolean
style?: CSSProperties | ((ColumnRenderProps & {
defaultStyle: CSSProperties
textValue?: string
width?: ColumnSize | null
} /@react-spectrum/s2:ColumnProps ColumnProps {
align?: 'start' | 'center' | 'end' = 'start'
allowsResizing?: boolean
allowsSorting?: boolean
children: ReactNode
className?: string | ((ColumnRenderProps & {
defaultClassName: string | undefined
})) => string
defaultWidth?: ColumnSize | null
id?: Key
isRowHeader?: boolean
maxWidth?: ColumnStaticSize | null
+ menu?: ReactNode
minWidth?: ColumnStaticSize | null
showDivider?: boolean
style?: CSSProperties | ((ColumnRenderProps & {
defaultStyle: CSSProperties
textValue?: string
width?: ColumnSize | null
} @react-spectrum/test-utils/@react-spectrum/test-utils:triggerLongPress triggerLongPress {
opts: {
element: HTMLElement
advanceTimer: (number) => void | Promise<unknown>
- pointerOpts?: Record<string, any>
+ pointerOpts?: {
+
}
+}
returnVal: undefined
} |
{(item) => <MenuItem>{item?.label}</MenuItem>} | ||
<Menu onAction={onMenuSelect} styles={style({minWidth: 128})}> | ||
{items.length > 0 && ( | ||
<MenuSection aria-label="Sort or resize?"> |
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.
not translated. also probably shouldn't be a question
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.
haha, it's a question because #7617 (comment)
width={150} | ||
minWidth={150} | ||
isRowHeader={column.isRowHeader} | ||
menu={ |
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.
Bit strange that this doesn't actually accept a <Menu>
. We should discuss options for the API here.
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, that's another option to handle it
was trying to answer #7617 (comment) as well
if it accepted a Menu, I guess we could do a custom renderer to inject our three menu items, still not a good way to ensure groups that make sense though
maybe people have to provide a menu with specific ids on certain children if they want resize/sort?
open to other ideas, not sold on what I currently have
Closes
Based on DNM - on hold: Implement Table Column custom actions
https://github.com/orgs/adobe/projects/19/views/32?visibleFields=%5B%22Title%22%2C%22Assignees%22%2C%22Status%22%2C4406231%2C25365423%2C3244152%2C5195578%2C17851188%2C30557450%2C3404283%2C%22Labels%22%5D&filterQuery=table&pane=issue&itemId=5984093
Some requirements we should consider, grouping multiple ways to sort. See acrobat.com's tables.
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: