Added SelectRowCommand and SelectColumnCommand. #295
Conversation
There was a problem hiding this comment.
LGTM for the most part :) My remarks:
- Some questions about reducing number of loops (minor).
- The
TableWalkerfor operating on rows isn't necessary. - More tests for
colspanandrowspanscenarios are needed even though we have 100% CC.- I'd remove the 'entire' from the column/row buttons' titles.
lang/contexts.json
Outdated
| @@ -4,11 +4,13 @@ | |||
| "Insert column left": "Label for the insert table column to the left of the current one button.", | |||
| "Insert column right": "Label for the insert table column to the right of the current one button.", | |||
| "Delete column": "Label for the delete table column button.", | |||
| "Select entire column": "Label for the select table entire column button.", | |||
There was a problem hiding this comment.
I think that we can drop "entire" from the title - this is for both commands.
There was a problem hiding this comment.
Yes, I think it can be "select column".
In fact, it should be "select column" or "select columns" depending on the selection, but the same applies to the other labels (e.g. delete column) and we ignored this for the moment.
src/commands/selectcolumncommand.js
Outdated
| * | ||
| * The command is registered by {@link module:table/tableediting~TableEditing} as the `'selectTableColumn'` editor command. | ||
| * | ||
| * To select the column containing the selected cell, execute the command: |
There was a problem hiding this comment.
Could you update the docs to reflect that it can select multiple columns? (also in the row command)
src/commands/selectcolumncommand.js
Outdated
| * | ||
| * The command is registered by {@link module:table/tableediting~TableEditing} as the `'selectTableColumn'` editor command. | ||
| * | ||
| * To select the column containing the selected cell, execute the command: |
There was a problem hiding this comment.
Could you update the docs to reflect that it can select multiple columns? (also in the row command)
src/commands/selectcolumncommand.js
Outdated
|
|
||
| for ( const cellInfo of new TableWalker( findAncestor( 'table', firstCell ) ) ) { | ||
| if ( cellInfo.column >= startColumn && cellInfo.column <= endColumn ) { | ||
| cellsToSelect.push( cellInfo.cell ); |
There was a problem hiding this comment.
I wonder if doing one less loop wouldn't be better. Here you can use model.createRangeOn() and have rangesToSelect array.
There was a problem hiding this comment.
However I don't see any performance gain on 20 columns so maybe it is a bit a stretch here.
src/commands/selectrowcommand.js
Outdated
| const table = findAncestor( 'table', referenceCells[ 0 ] ); | ||
| const cellsToSelect = []; | ||
|
|
||
| for ( const cellInfo of new TableWalker( table, { startRow: rowIndexes.first, endRow: rowIndexes.last } ) ) { |
There was a problem hiding this comment.
I'd go with iterating over table.getChildren() or event for loop and retrieving rows by index from table. Then for every row select all children (table cells). The TableWalker is cool to deal with columns and spans when needed but here we have simpler solution as operations on row indexes is straightforward (those maps 1:1 even with rowspans). As opposite to working with columns indexes where there's no 1:1 mapping to columns.
tests/commands/selectrowcommand.js
Outdated
| // + +----+----+----+----+ | ||
| // | | 31 | 32 | 33 | 34 | | ||
| // +----+----+----+----+----+ | ||
| setData( model, modelTable( [ |
There was a problem hiding this comment.
Similarly to the comment in column comand - please add more cases with rowspan.
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
Co-Authored-By: Maciej <[email protected]>
Suggested merge commit message (convention)
Feature: "Select entire column/row" added to table row/column dropdowns. Closes ckeditor/ckeditor5#6500.
Additional information