Skip to content
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

fix(table): set min-width to <col> #5464

Merged
merged 5 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/two-rats-watch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@tiptap/extension-table": patch
---

enforce cellMinWidth even on column not resized by the user, fixes #5435
12 changes: 10 additions & 2 deletions demos/src/Nodes/Table/React/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,22 @@ context('/src/Nodes/Table/React/', () => {
})
})

it('sets the minimum width on the colgroups by default (3x1)', () => {
cy.get('.tiptap').then(([{ editor }]) => {
editor.commands.insertTable({ cols: 3, rows: 1, withHeaderRow: false })

cy.get('.tiptap').find('col').invoke('attr', 'style').should('eq', 'min-width: 25px;')
})
})

it('generates correct markup for a table (1x1)', () => {
cy.get('.tiptap').then(([{ editor }]) => {
editor.commands.insertTable({ cols: 1, rows: 1, withHeaderRow: false })

const html = editor.getHTML()

expect(html).to.equal(
'<table style="min-width: 25px"><colgroup><col></colgroup><tbody><tr><td colspan="1" rowspan="1"><p></p></td></tr></tbody></table>',
'<table style="min-width: 25px"><colgroup><col style="min-width: 25px"></colgroup><tbody><tr><td colspan="1" rowspan="1"><p></p></td></tr></tbody></table>',
)
})
})
Expand All @@ -75,7 +83,7 @@ context('/src/Nodes/Table/React/', () => {
const html = editor.getHTML()

expect(html).to.equal(
'<table style="min-width: 25px"><colgroup><col></colgroup><tbody><tr><th colspan="1" rowspan="1"><p></p></th></tr></tbody></table>',
'<table style="min-width: 25px"><colgroup><col style="min-width: 25px"></colgroup><tbody><tr><th colspan="1" rowspan="1"><p></p></th></tr></tbody></table>',
)
})
})
Expand Down
16 changes: 14 additions & 2 deletions demos/src/Nodes/Table/Vue/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,23 @@ context('/src/Nodes/Table/Vue/', () => {
})
})

it('sets the minimum width on the colgroups by default (3x1)', () => {
cy.get('.tiptap').then(([{ editor }]) => {
editor.commands.insertTable({ cols: 3, rows: 1, withHeaderRow: false })

cy.get('.tiptap').find('col').invoke('attr', 'style').should('eq', 'min-width: 25px;')
})
})

it('generates correct markup for a table (1x1)', () => {
cy.get('.tiptap').then(([{ editor }]) => {
editor.commands.insertTable({ cols: 1, rows: 1, withHeaderRow: false })

const html = editor.getHTML()

expect(html).to.equal('<table style="min-width: 25px"><colgroup><col></colgroup><tbody><tr><td colspan="1" rowspan="1"><p></p></td></tr></tbody></table>')
expect(html).to.equal(
'<table style="min-width: 25px"><colgroup><col style="min-width: 25px"></colgroup><tbody><tr><td colspan="1" rowspan="1"><p></p></td></tr></tbody></table>',
)
})
})

Expand All @@ -72,7 +82,9 @@ context('/src/Nodes/Table/Vue/', () => {

const html = editor.getHTML()

expect(html).to.equal('<table style="min-width: 25px"><colgroup><col></colgroup><tbody><tr><th colspan="1" rowspan="1"><p></p></th></tr></tbody></table>')
expect(html).to.equal(
'<table style="min-width: 25px"><colgroup><col style="min-width: 25px"></colgroup><tbody><tr><th colspan="1" rowspan="1"><p></p></th></tr></tbody></table>',
)
})
})

Expand Down
56 changes: 31 additions & 25 deletions packages/extension-table/src/TableView.ts
Original file line number Diff line number Diff line change
@@ -1,49 +1,55 @@
// @ts-nocheck
import { Node as ProseMirrorNode } from '@tiptap/pm/model'
import { NodeView } from '@tiptap/pm/view'

import { getColStyleDeclaration } from './utilities/colStyle.js'

export function updateColumns(
node: ProseMirrorNode,
colgroup: Element,
table: Element,
colgroup: HTMLTableColElement, // <colgroup> has the same prototype as <col>
table: HTMLTableElement,
cellMinWidth: number,
overrideCol?: number,
overrideValue?: any,
overrideValue?: number,
) {
let totalWidth = 0
let fixedWidth = true
let nextDOM = colgroup.firstChild
const row = node.firstChild

for (let i = 0, col = 0; i < row.childCount; i += 1) {
const { colspan, colwidth } = row.child(i).attrs

for (let j = 0; j < colspan; j += 1, col += 1) {
const hasWidth = overrideCol === col ? overrideValue : colwidth && colwidth[j]
const cssWidth = hasWidth ? `${hasWidth}px` : ''
if (row !== null) {
for (let i = 0, col = 0; i < row.childCount; i += 1) {
const { colspan, colwidth } = row.child(i).attrs

totalWidth += hasWidth || cellMinWidth
for (let j = 0; j < colspan; j += 1, col += 1) {
const hasWidth = overrideCol === col ? overrideValue : (colwidth && colwidth[j]) as number | undefined
const cssWidth = hasWidth ? `${hasWidth}px` : ''

if (!hasWidth) {
fixedWidth = false
}
totalWidth += hasWidth || cellMinWidth

if (!nextDOM) {
colgroup.appendChild(document.createElement('col')).style.width = cssWidth
} else {
if (nextDOM.style.width !== cssWidth) {
nextDOM.style.width = cssWidth
if (!hasWidth) {
fixedWidth = false
}

nextDOM = nextDOM.nextSibling
if (nextDOM === null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the nit, would prefer to keep this as

Suggested change
if (nextDOM === null) {
if (!nextDOM) {

Just in case it is undefined, and to reduce differentiation from the prior behavior

const colElement = document.createElement('col')

colElement.style.setProperty(...getColStyleDeclaration(cellMinWidth, hasWidth))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While clever, I would prefer to be explicit around this, so that it is easy to understand what is happening:

Suggested change
colElement.style.setProperty(...getColStyleDeclaration(cellMinWidth, hasWidth))
const [propertyKey, propertyValue] = getColStyleDeclaration(cellMinWidth, hasWidth)
colElement.style.setProperty(propertyKey, propertyValue)

Same thing in the next few lines

colgroup.appendChild(colElement)
} else {
if ((nextDOM as HTMLTableColElement).style.width !== cssWidth) {
(nextDOM as HTMLTableColElement).style.setProperty(...getColStyleDeclaration(cellMinWidth, hasWidth))
}

nextDOM = nextDOM.nextSibling
}
}
}
}

while (nextDOM) {
const after = nextDOM.nextSibling

nextDOM.parentNode.removeChild(nextDOM)
nextDOM.parentNode?.removeChild(nextDOM)
nextDOM = after
}

Expand All @@ -61,13 +67,13 @@ export class TableView implements NodeView {

cellMinWidth: number

dom: Element
dom: HTMLDivElement

table: Element
table: HTMLTableElement

colgroup: Element
colgroup: HTMLTableColElement

contentDOM: Element
contentDOM: HTMLTableSectionElement

constructor(node: ProseMirrorNode, cellMinWidth: number) {
this.node = node
Expand Down
14 changes: 14 additions & 0 deletions packages/extension-table/src/utilities/colStyle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export function getColStyleDeclaration(minWidth: number, width: number | undefined): [string, string] {
if (width) {
// apply the stored width unless it is below the configured minimum cell width
return ['width', `${Math.max(width, minWidth)}px`]
}

// set the minimum with on the column if it has no stored width
return ['min-width', `${minWidth}px`]

}

export function getColStyle(minWidth: number, width: number): string {
return getColStyleDeclaration(minWidth, width).join(': ')
}
Copy link
Contributor

@nperez0111 nperez0111 Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is abstracting over a couple of if statements, I'm not sure it warrants the complexity it adds.

I would honestly prefer for there to be a single function here, either stick to the string version or the array version, both just adds complexity and indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extracted them because the logic was originally copy-pasted in both the create and the update code, I can remove the getColStyle function and do the concatenation in the table creation function directly instead of having a function in this file

30 changes: 25 additions & 5 deletions packages/extension-table/src/utilities/createColGroup.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { DOMOutputSpec, Node as ProseMirrorNode } from '@tiptap/pm/model'

import { getColStyle } from './colStyle.js'

export type ColGroup = {
colgroup: DOMOutputSpec
tableWidth: string
tableMinWidth: string
} | Record<string, never>;

/**
* Creates a colgroup element for a table node in ProseMirror.
*
Expand All @@ -9,12 +17,22 @@ import { DOMOutputSpec, Node as ProseMirrorNode } from '@tiptap/pm/model'
* @param overrideValue - (Optional) The width value to use for the overridden column.
* @returns An object containing the colgroup element, the total width of the table, and the minimum width of the table.
*/
export function createColGroup(
node: ProseMirrorNode,
cellMinWidth: number,
): ColGroup
export function createColGroup(
node: ProseMirrorNode,
cellMinWidth: number,
overrideCol: number,
overrideValue: number,
): ColGroup
export function createColGroup(
node: ProseMirrorNode,
cellMinWidth: number,
overrideCol?: number,
overrideValue?: any,
) {
overrideValue?: number,
): ColGroup {
let totalWidth = 0
let fixedWidth = true
const cols: DOMOutputSpec[] = []
Expand All @@ -28,16 +46,18 @@ export function createColGroup(
const { colspan, colwidth } = row.child(i).attrs

for (let j = 0; j < colspan; j += 1, col += 1) {
const hasWidth = overrideCol === col ? overrideValue : colwidth && colwidth[j]
const cssWidth = hasWidth ? `${hasWidth}px` : ''
const hasWidth = overrideCol === col ? overrideValue : colwidth && colwidth[j] as number | undefined

totalWidth += hasWidth || cellMinWidth

if (!hasWidth) {
fixedWidth = false
}

cols.push(['col', cssWidth ? { style: `width: ${cssWidth}` } : {}])
cols.push([
'col',
{ style: getColStyle(cellMinWidth, hasWidth) },
])
}
}

Expand Down