-
-
Notifications
You must be signed in to change notification settings - Fork 755
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
Table cells merging #2666
Table cells merging #2666
Conversation
…lls-merging-poc-2 # Conflicts: # packages/table/src/transforms/insertTableRow.ts # yarn.lock
…lls-merging-poc-2
…lls-merging-poc-2 # Conflicts: # apps/www/src/registry/default/plate-ui/table-element.tsx # packages/table/src/components/TableCellElement/useTableCellElementResizable.ts # packages/table/src/components/TableCellElement/useTableCellElementState.ts # yarn.lock
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
convert AnyObject from interface to type convert TElement from interface to type convert TTest from interface to type remove tsx package & dev script
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.
- added
disableMergeCells
plugin option, it is important to keep the merging cell logic decoupled so we can prevent running that code when disabled. Also important for maintenance, readability and testing. I've created amerge
folder to start with. TTableCellElement
: rowIndex and colIndex should not be stored on the element. These can be retrieved using findNodePath, and could even be stored in a WeakMap. I see many colSpan and rowSpan additions in the tests. We should keep the previous default: keep undefined when value is 1. SorowIndex={1} colIndex={1} rowSpan={1} colSpan={1}
are redundant.- deleteColumn, deleteRow, insertTableColumn, insertTableRow: decouple and don't call the new logic when merging is disabled
- last thing to do is to decouple the state from the UI since it's not versioned. I'll take care of that.
Thanks again for your contribution @dimaanj, I understand that you may be pressed for time regarding this PR. I will try to find some time this month to finalize any remaining point.
if (!colSizes.includes(0)) { | ||
colSizes.push('100%' as any); | ||
} | ||
const tableWidth = colSizes.reduce((acc, cur) => acc + cur, 0); |
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.
We should support auto width when merging is disabled.
readOnly, | ||
selected, | ||
hovered, | ||
hoveredLeft, | ||
rowSize, | ||
borders, | ||
isSelectingCell, | ||
cellRef, |
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.
should come from useTableCellElement
if (cellRef.current && hoveredColIndex === null && cellOffsets) { | ||
const cellOffset = cellRef.current.offsetLeft; | ||
const startColIndex = getClosest(cellOffset, cellOffsets); | ||
|
||
startCIndex.current = startColIndex; | ||
endColIndex.current = startColIndex + cellElement.colSpan - 1; | ||
} |
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.
ref and its relative logic is DOM related, so it should be moved to useTableCellElement
export const getTableColumnCount = (tableNode: TElement) => { | ||
return (tableNode.children as TElement[])?.[0]?.children?.length ?? 0; |
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.
New logic should be called only when merging not disabled
* colSizes with default widths. Since colSizes should always return valid widths | ||
* of the columns for table cells merging feature. |
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.
0-filled array was used for auto layout. We need to keep support for it when merging is disabled.
Follow-up of #2590