-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
#6667: Table heading rows should be properly updated after removing rows as a side effect of merging cells #6764
Conversation
…a side effect of merging cells.
I've almost merged it with some changes but I had to revert it by force push (sorry for that). Unfortunatelly it doesn't play well with undo (current master works): the broken structure is a bug but the other thing is that number of heading rows isn't restored properly or isn't well handled. I had similar case but i can't reproduce it without that broken structure. Could you check why this happens in this PR? Maybe a merge with master will be enough. The other hint is to check it how remove rows command handle this. |
…t table state for update).
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.
Minor changes. Other that that looks good :+1: .
@@ -83,6 +80,8 @@ export default class MergeCellCommand extends Command { | |||
const model = this.editor.model; | |||
const doc = model.document; | |||
const tableCell = getTableCellsContainingSelection( doc.selection )[ 0 ]; | |||
const table = findAncestor( 'table', tableCell ); |
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 be moved to if
below - AFAICS it will not be needed in other cases.
@@ -57,40 +57,25 @@ export default class MergeCellsCommand extends Command { | |||
updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer ); | |||
updateNumericAttribute( 'rowspan', mergeHeight, firstTableCell, writer ); | |||
|
|||
const emptyRows = []; |
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.
Maybe emptyRowsIndexes
- just to be ultra precise.
} | ||
|
||
emptyRows.reverse().forEach( row => tableUtils.removeRows( table, { at: row, batch: writer.batch } ) ); |
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.
And since a table
is only needed here maybe a wrap in if( emptyRows.length )
? But IDK for sure - this can be left as is.
Co-authored-by: Maciej <[email protected]>
Suggested merge commit message (convention)
Fix (table): Table heading rows should be properly updated after removing rows as a side effect of merging cells. Closes #6667.
Additional information