Add resize table column balloon editor #16104
Conversation
Mati365
changed the title
Mati365
changed the title
Focus is placed outside of the balloon after selecting the “Resize column” option and there’s no way to insert a column size value using keyboard onlySteps
ResultFocus is outside of the “Column width in pixels” field and it’s not possible to insert a number into the field without using mouse: 1.mp4 |
Table size balloon is left out after removing the tableSteps
ResultThe balloon with table size is not removed: 2.mp4 |
|
#16104 (comment) and #16104 (comment) look fixed 👍 |
It's not possible to resize a column right after table insertionSteps
ResultThe option is greyed out. Resizing the table using resize handler activates the option: 3.mp4 |
Resizing using the "Resize column" option doesn't work with Track ChangesSteps
ResultThe "Resize column" option is greyed out even though it's active with TC off: 4.mp4 |
Table column width changes when the column is selectedSteps
ResultThe column gets narrower when selected: 5.mp4NoteIt's a regression - not reproducible on master |
The error when entering an incorrect number is misleadingThe error suggests that it is possible to enter a number with a comma/dot (depends on the browser) - but this is not possible.Steps:
Result:An error appears that the entered value is invalid resize.movNotes:When entering an incorrect value and switching focus from the input to something else, the input header overlaps with the incorrect value overlaps.mov |
|
@kubaklatt / @charlttsie All reported issues should be fixed. Please switch to |
|
We've finished testing with @kubaklatt and @marcelmroz and the PR looks good. We haven't found anything new, while the previously reported issues don't reproduce anymore. Thanks @Mati365 for the quick fixes! 🎉 |
Mati365
force-pushed
the
ck/6023
branch
2 times, most recently
from
9f68734
to
6fadb2e
Compare
There was a problem hiding this comment.
- Changing the size of the furthest right column sets different result:
Screen.Recording.2024-04-10.at.12.49.18.mov
- The "Resize column" button is always visible in the dropdown, even if the table column resize feature is not loaded in the editor.
- I'd try to implement a live preview when changing the column widths. Without it user has to guess a few times until they find the expected size. It should work like the other table properties, so apply only view changes until the change is applied and after committing the new value, propagate change to the model - this way we only have one undo step.
|
There was a problem hiding this comment.
Overall, it's really cool it's all working and that you did quite a lot of cleaning in the TableColumnResizeEditing plugin 👏🏻
Besides some necessary cleanup that I wrote about below, I noticed two more issues:
- When a selected cell spans through a few columns, the width of the last one is shown in the form and resized if you change the width. I think it should be the first one.
Screen.Recording.2024-04-12.at.12.20.25.mov
- A problem with undo. If you change the width twice and undo once, then open the resize form, then you'll see the wrong value.
Screen.Recording.2024-04-12.at.12.25.34.mov
packages/ckeditor5-table/src/tablecolumnresize/commands/tableresizecolumncommand.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-table/src/tablecolumnresize/commands/tableresizecolumncommand.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-table/src/tablecolumnresize/tablecolumnresizeui.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-table/src/tablecolumnresize/tablecolumnresizeutils.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-table/src/tablecolumnresize/commands/tableresizecolumncommand.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-table/src/tablecolumnresize/utils/getsmallestcolumnresizer.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-table/src/tablecolumnresize/tablecolumnresizeutils.ts
Show resolved
Hide resolved
packages/ckeditor5-table/src/tablecolumnresize/tablecolumnresizeutils.ts
Outdated
Show resolved
Hide resolved
Rest of CR remarks should be fixed. Can you check again? |
|
No changes, rebased. |
Maximum column size allowed value is 1px higher from the size that can be setSteps
ResultThe allowed value was 1px higher from the actual size that's been set: 1px.mp4 |
For a single-column table, minimum column size allowed is 1px higher from the size that can be setSteps
ResultThe column used to have 40px but now has 39px: minimum-size-one-column.mp4 |
For a single-column table, the value is sometimes set to 1px lower from what's been insertedSeems related to: #16104 (comment), but this scenario doesn't involve the validation issue Steps
ResultSometimes, but not always (see 460px at 00:12 in the screencast), the value after reopening the resize balloon differs from what's been inserted: size-1px.mp4 |
|
FYI: #16249 - Table column size changes when resizing browser window after column resize was used The above issue also occurs on master, but it affects the pull request in such a way that a column width smaller than allowed can be achieved by resizing the browser window: 
|
|
FYI: #16251 - Clicking on a resize handle in a nested table extends the tables to the full editor width The above issue also reproduces when resizing using the balloon: resize-balloon-nested.mp4 |
WalkthroughThe updates to the CKEditor5's table package focus on enhancing table column resizing capabilities. Changes include the introduction of new UI components, commands, and utilities for resizing table columns, alongside necessary dependency adjustments and code refactoring. These modifications aim to improve user interaction with table columns in the CKEditor environment, making the resizing process more intuitive and precise. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (22)
Files skipped from review due to trivial changes (3)
Additional comments not posted (45)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| public prepareColumnResize( resizerElement: ViewElement ): ResizingData | null { | ||
| const { editing } = this.editor; | ||
|
|
||
| const viewTable = resizerElement.findAncestor( 'table' )!; | ||
| const editingView = editing.view; | ||
| const columnWidthsInPx = calculateResizerColumnWidth( this.editor, resizerElement ); | ||
|
|
||
| if ( !columnWidthsInPx ) { | ||
| return null; | ||
| } | ||
|
|
||
| // Insert colgroup for the table that is resized for the first time. | ||
| if ( !Array.from( viewTable.getChildren() ).find( viewCol => viewCol.is( 'element', 'colgroup' ) ) ) { | ||
| editingView.change( viewWriter => { | ||
| insertColgroupElement( viewWriter, columnWidthsInPx, viewTable ); | ||
| } ); | ||
| } | ||
|
|
||
| const resizingData = getResizingData( this.editor, resizerElement, columnWidthsInPx ); | ||
|
|
||
| // At this point we change only the editor view - we don't want other users to see our changes yet, | ||
| // so we can't apply them in the model. | ||
| this.editor.editing.view.change( writer => applyResizingAttributesToTable( writer, viewTable, resizingData ) ); | ||
|
|
||
| return resizingData; |
There was a problem hiding this comment.
Handle potential exceptions in DOM operations.
The method prepareColumnResize performs several DOM manipulations without any exception handling. Consider adding try-catch blocks around these operations to handle potential errors gracefully, especially since DOM operations can fail due to various reasons like incorrect selectors or unexpected HTML structure changes.
| public endResize( resizingData: ResizingData, readOnly?: boolean ): void { | ||
| const editor = this.editor; | ||
| const editingView = editor.editing.view; | ||
|
|
||
| const { | ||
| viewResizer, | ||
| modelTable, | ||
| viewFigure, | ||
| viewColgroup | ||
| } = resizingData.elements; | ||
|
|
||
| const tableColumnGroup = getColumnGroupElement( modelTable ); | ||
| const viewColumns: Array<ViewElement> = Array | ||
| .from( viewColgroup!.getChildren() ) | ||
| .filter( ( column: ViewNode ): column is ViewElement => column.is( 'view:element' ) ); | ||
|
|
||
| const columnWidthsAttributeOld = tableColumnGroup ? | ||
| getTableColumnsWidths( tableColumnGroup )! : | ||
| null; | ||
|
|
||
| const columnWidthsAttributeNew = viewColumns.map( column => column.getStyle( 'width' ) ); | ||
|
|
||
| const isColumnWidthsAttributeChanged = !isEqual( columnWidthsAttributeOld, columnWidthsAttributeNew ); | ||
|
|
||
| const tableWidthAttributeOld = modelTable.getAttribute( 'tableWidth' ) as string; | ||
| const tableWidthAttributeNew = viewFigure.getStyle( 'width' )!; | ||
|
|
||
| const isTableWidthAttributeChanged = tableWidthAttributeOld !== tableWidthAttributeNew; | ||
|
|
||
| if ( isColumnWidthsAttributeChanged || isTableWidthAttributeChanged ) { | ||
| if ( readOnly ) { | ||
| // In read-only mode revert all changes in the editing view. The model is not touched so it does not need to be restored. | ||
| // This case can occur if the read-only mode kicks in during the resizing process. | ||
| editingView.change( writer => { | ||
| // If table had resized columns before, restore the previous column widths. | ||
| // Otherwise clean up the view from the temporary column resizing markup. | ||
| if ( columnWidthsAttributeOld ) { | ||
| for ( const viewCol of viewColumns ) { | ||
| writer.setStyle( 'width', columnWidthsAttributeOld.shift()!, viewCol ); | ||
| } | ||
| } else { | ||
| writer.remove( viewColgroup! ); | ||
| } | ||
|
|
||
| if ( isTableWidthAttributeChanged ) { | ||
| // If the whole table was already resized before, restore the previous table width. | ||
| // Otherwise clean up the view from the temporary table resizing markup. | ||
| if ( tableWidthAttributeOld ) { | ||
| writer.setStyle( 'width', tableWidthAttributeOld, viewFigure ); | ||
| } else { | ||
| writer.removeStyle( 'width', viewFigure ); | ||
| } | ||
| } | ||
|
|
||
| // If a table and its columns weren't resized before, | ||
| // prune the remaining common resizing markup. | ||
| if ( !columnWidthsAttributeOld && !tableWidthAttributeOld ) { | ||
| writer.removeClass( | ||
| 'ck-table-resized', | ||
| [ ... viewFigure.getChildren() as IterableIterator<ViewElement> ].find( element => element.name === 'table' )! | ||
| ); | ||
| } | ||
| } ); | ||
| } else { | ||
| editor.execute( 'resizeTableWidth', { | ||
| table: modelTable, | ||
| tableWidth: `${ toPrecision( tableWidthAttributeNew ) }%`, | ||
| columnWidths: columnWidthsAttributeNew | ||
| } ); | ||
| } | ||
| } | ||
|
|
||
| editingView.change( writer => { | ||
| writer.removeClass( 'ck-table-column-resizer__active', viewResizer ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Consider refactoring to improve readability.
The method endResize is quite lengthy and handles multiple responsibilities. Consider breaking it down into smaller, more focused methods to improve readability and maintainability. For example, handling view restoration and command execution could be separated into different methods.
| function insertColgroupElement( | ||
| viewWriter: DowncastWriter, | ||
| columnWidthsInPx: Array<number>, | ||
| viewTable: ViewElement | ||
| ): void { | ||
| const colgroup = viewWriter.createContainerElement( 'colgroup' ); | ||
|
|
||
| for ( let i = 0; i < columnWidthsInPx.length; i++ ) { | ||
| const viewColElement = viewWriter.createEmptyElement( 'col' ); | ||
| const columnWidthInPc = `${ toPrecision( columnWidthsInPx[ i ] / sumArray( columnWidthsInPx ) * 100 ) }%`; | ||
|
|
||
| viewWriter.setStyle( 'width', columnWidthInPc, viewColElement ); | ||
| viewWriter.insert( viewWriter.createPositionAt( colgroup, 'end' ), viewColElement ); | ||
| } | ||
|
|
||
| viewWriter.insert( viewWriter.createPositionAt( viewTable, 0 ), colgroup ); | ||
| } |
There was a problem hiding this comment.
Validate input parameters for safety.
The function insertColgroupElement does not validate the input parameters columnWidthsInPx and viewTable. Ensure that these parameters are validated to prevent runtime errors if unexpected values are passed.
| async function createEditor( config ) { | ||
| return ClassicTestEditor.create( element, { | ||
| plugins: [ TableEditing, TableUI ], | ||
| ...config | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Refactor to improve code reuse and maintainability.
The createEditor function is a good refactor that simplifies editor creation with configurable plugins. Consider using this pattern in other test files as well to improve code reuse and maintainability across the project.
| /* | ||
| * Fixes the problem where surrounding cells cover the focused cell's border. | ||
| * It does not fix the problem in all places but the UX is improved. | ||
| * See https://github.com/ckeditor/ckeditor5-table/issues/29. | ||
| */ | ||
| border-color: var(--ck-color-cell-border); |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that the CSS variable --ck-color-cell-border is used in tableediting.css but there is no definition for it within the theme files. This suggests that the variable might not be defined, which could lead to inconsistencies in the theme's appearance. It is recommended to define this variable or verify its definition in a global stylesheet if it exists.
packages/ckeditor5-theme-lark/theme/ckeditor5-table/tableediting.css: Variable used but not defined in the theme files.
Analysis chain
Ensure the new border color variable is defined and consistent with the theme.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of --ck-color-cell-border in the theme files to ensure consistency.
rg --type css '--ck-color-cell-border' packages/ckeditor5-theme-lark/theme/
Length of output: 500
Script:
#!/bin/bash
# Corrected script to search for the definition of --ck-color-cell-border in the theme files to ensure consistency.
rg -e '--ck-color-cell-border' --type css packages/ckeditor5-theme-lark/theme/
Length of output: 193
It is not possible to resize a table inside another table in a specific caseSteps
ResultIt is not possible to resize a 1x1 table, no matter what value we enter. The suggestions as to what value we can enter are wrong in this case. table.mov |
|
@Mati365 |
Suggested merge commit message (convention)
Feature (table): Add "Resize Column" option in the table editor toolbar, offering an alternative to drag-and-drop resizing.
Additional information
Closes https://github.com/cksource/ckeditor5-commercial/issues/6023
Part of https://github.com/cksource/ckeditor5-commercial/pull/6133
Related to #16140
Screens:
Summary by CodeRabbit
New Features
Bug Fixes
parseFloat.Refactor
Style
Documentation