-
-
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
I/6122: Handling spanned table cells in pasting scenarios #7226
Conversation
…s are smaller than pasted table.
…ed table to selection area.
packages/ckeditor5-table/src/commands/setheadercolumncommand.js
Outdated
Show resolved
Hide resolved
const cropDimensions = { | ||
startRow: 0, | ||
startColumn: 0, | ||
endRow: Math.min( selectionHeight - 1, pasteHeight - 1 ), | ||
endColumn: Math.min( selectionWidth - 1, pasteWidth - 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.
We have multiple different naming conventions for startRow
/endRow
vs firstRow
/lastRow
. I think we should stick to one notation.
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.
That's a good point. Unfortunately we have both in some API points and I'm not sure which one is better.
In my last commit I addressed most issues. Still to discuss:
Also I didn't reviewed tests. |
Did you checked it after latest fixes? |
Please - if you fix something - add tests. I'm debugging what's happening as there was no new tests and I'm thinking why this is working now. Test are needed for changed logic and bug fixes. I'll add those test. |
Steps to reproduce:
In scenario A pasted content is a single cell, whereas in B it remembers that initial table width was 2 columns. Not sure if this is incorrect, but it's a bit counterintuitive, because what you see when copying in both cases, is only a one-cell-table: |
Co-authored-by: Kuba Niegowski <[email protected]>
Moved here: #7246. |
The bug is here :) Already reported: #6609. Would be nice to fix it eventually. |
Suggested merge commit message (convention)
Internal (table): Handle spanned table cells in pasting scenarios. Closes #6122.
Fix (table): Setting column as header will now split it col-spaned cells. Closes #6658.
Additional information
This PR: