-
Notifications
You must be signed in to change notification settings - Fork 14
[RHCLOUD-43454] Updating data-view docs and adding compound options for Expandable, Sticky and Draggable #575
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
base: main
Are you sure you want to change the base?
Conversation
22f15f0 to
cb0a2f4
Compare
packages/module/patternfly-docs/content/extensions/data-view/examples/Table/Table.md
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/data-view/examples/Table/Table.md
Outdated
Show resolved
Hide resolved
radekkaluzik
left a comment
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
nicolethoen
left a comment
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 is looking great! I have a number of questions though.
I want to note that the draggable solution here inherits the same lack of keyboard accessibility that PF's existing draggable table solution has. I bet claude could help brainstorm a fix there.. I'll double check PF's table backlog to see if there's a plan for remedying it in our base table component.
|
|
||
| export const BasicExample: FunctionComponent = () => ( | ||
| <DataViewTable aria-label='Repositories table' ouiaId={ouiaId} columns={columns} rows={rows} /> | ||
| <DataViewTable aria-label='Repositories table' ouiaId={ouiaId} columns={columns} rows={rows} isExpandable={true}/> |
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.
Was this change intended? I think it can be reverted.
| {(rowIsObject ? row.row : row).map((cell, colIndex) => { | ||
| const cellIsObject = isDataViewTdObject(cell); | ||
| return ( | ||
| <Tbody key={rowIndex} isExpanded={isRowExpanded} onDragOver={onDragOver} onDrop={onDropTbody} onDragLeave={onDragLeave}> |
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 this can be tightened up a bit so that the separate <Tbody> wrapper should only apply when isExpandable || isDraggable is true.
I was tinkering with claude a bit and claude suggested something like the following can handle the conditional:
const renderedRows = useMemo(() => {
const needsSeparateTbody = isExpandable || isDraggable;
return rows.map((row, rowIndex) => {
// ... calculate row data ...
if (needsSeparateTbody) {
return (
<Tbody key={rowIndex} isExpanded={isRowExpanded} ...>
<Tr ...>{/* cells */}</Tr>
{/* expandable content */}
</Tbody>
);
} else {
// Original structure for basic tables
return (
<Tr key={rowIndex} ...>
{/* cells */}
</Tr>
);
}
});
}, [...]);
// And in the return:
if (needsSeparateTbody) {
return <Table ...>{activeBodyState || renderedRows}</Table>;
} else {
return <Table ...>{activeBodyState || <Tbody>{renderedRows}</Tbody>}</Table>;
}
| } | ||
| }; | ||
|
|
||
| const onDrop: TrProps['onDrop'] = (evt) => { |
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.
The draggable rows feature currently reorders rows in the DOM but doesn't provide a way for parent components to be notified of the new order. This means users cannot persist the reordered state or update their data model.
Let's add an additional prop which is a callback that is executed when the order is updated. something like:
export interface DataViewTableBasicProps extends Omit<TableProps, 'onSelect' | 'rows'> {
// ... existing props
/** Callback fired when rows are reordered via drag and drop */
onReorder?: (newOrder: string[], oldOrder: string[]) => void;
}
then you would want to leverage that new prop in useDraggableRows:
export interface UseDraggableRowsProps {
rows: DataViewTr[];
tableRef: React.RefObject<HTMLTableElement>;
onReorder?: (newOrder: string[], oldOrder: string[]) => void;
}
export const useDraggableRows = ({
rows,
tableRef,
onReorder
}: UseDraggableRowsProps): UseDraggableRowsReturn => {
// ... existing code ...
const onDrop: TrProps['onDrop'] = (evt) => {
if (isValidDrop(evt) && tableRef.current) {
const tbodyNodes = Array.from(tableRef.current.querySelectorAll('tbody'));
const finalOrder = tbodyNodes
.map(tbody => tbody.querySelector('tr[id]'))
.filter((tr): tr is HTMLTableRowElement => tr !== null)
.map(tr => tr.id);
const oldOrder = itemOrder;
setItemOrder(finalOrder);
// Notify parent component
if (onReorder && JSON.stringify(oldOrder) !== JSON.stringify(finalOrder)) {
onReorder(finalOrder, oldOrder);
}
} else {
onDragCancel();
}
};
// ... rest of implementation
};
| isExpanded: isRowExpanded && expandedColIndex === colIndex, | ||
| expandId: `expandable-${rowIndex}`, | ||
| onToggle: () => { | ||
| console.log(`toggled compound expand for row ${rowIndex}, column ${colIndex}`); // eslint-disable-line no-console |
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.
can this be removed?
| export interface UseDraggableRowsProps { | ||
| rows: DataViewTr[]; | ||
| tableRef: React.RefObject<HTMLTableElement>; | ||
| isDraggable?: boolean; |
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 this used anywhere in this hook?
|
|
||
| ### Interactive example | ||
| - Interactive example show how the different composable options work together. | ||
| - By toggling the toggles you can switch between them and observe the bahaviour |
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.
*behaviour (or behavior)
| }} | ||
| > | ||
| {cellIsObject ? cell.cell : cell} | ||
| <GripVerticalIcon /> |
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 was about to note that this icon needs an accessible label but realized that I don't think you should need to declare the icon directly.
can you reuse PF's table API to get the correct drag handle + behavior more out of the box?
something like:
{isDraggable && (
<Td
key={`drag-${rowIndex}`}
draggableRow={{
id: `draggable-row-${rowIds[rowIndex]}`
}}
/>
)}
i'm looking at https://www.patternfly.org/components/table/#draggable-row-table
may be worth seeing if there are more out of the box capaibilities you're not yet leveraging.
As part of task RHCLOUD-43454
Assisted-by: Claude Code