Switch to dynamically calculated cell map #12288
Conversation
…e notification (the code should always reach change assignment at the end of postfixer loop).
There was a problem hiding this comment.
Overall good job with the refactoring. This once unreadable postfixer is now becoming quite nice!
I already discussed the changes with @Dumluregn f2f and we agreed that I'll apply the changes as a part of R.
| @@ -206,80 +192,48 @@ export default class TableColumnResizeEditing extends Plugin { | |||
| // (1) Adjust the `columnWidths` attribute to guarantee that the sum of the widths from all columns is 100%. | |||
There was a problem hiding this comment.
Here we go again with the getChangedTables() method name (we already changed it once 😅). Looking fresh at implementation it should be called like getChangedResizedTables() - current name can imply that it's just about any table.
|
|
||
| isColumnInsertionHandled = true; | ||
| changed = true; |
There was a problem hiding this comment.
The changed = true; line seems to be redundant for me. The point of entire if ( columnsCountDelta != 0 ) block is to recalculate columnWidths. As such it should always end up in this final assignment.
There was a problem hiding this comment.
I removed both lines and no unit test fails which further convince me in this assumption.
| const isColumnInsertion = previousColumn < column; | ||
| const isColumnDeletion = previousColumn > column; | ||
| // If the column count in the table changed, adjust the widths of the affected columns. | ||
| const operationType = currentColumnsDelta > 0 ? 'insert' : 'remove'; |
There was a problem hiding this comment.
There are just two possible values for this variable and you're using it only for algorithm.
I'd say that a boolean value would be a better choice here. This would result with easier conditions later on. You'd just do
const isAddedColumn = currentColumnsDelta > 0;
And it would be just:
if ( isAddedColumn ) {
// ...
} else {
// ...
}Instead of:
if ( operationType == 'insert' ) {
// ...
}
if ( operationType == 'remove' ) {
// ...
}
|
Pushed the changes. I'll wait for CI with merge. |
Suggested merge commit message (convention)
Internal (table): Calculate the modified cells positions dynamically instead of keeping the static map in the plugin. Closes #12266.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.