TableLayout plugin.
#17931
TableLayout plugin.
#17931
Conversation
Internal (table): Fix missing tableType attribute in cropTableToDimensions output.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
packages/ckeditor5-table/src/commands/setheadercolumncommand.ts
Outdated
Show resolved
Hide resolved
| * editor.execute( 'insertTableLayout', { rows: 20, columns: 5 } ); | ||
| * ``` | ||
| */ | ||
| export default class InsertTableLayoutCommand extends InsertTableCommand { |
There was a problem hiding this comment.
I'd suggest extending a Command and copy-paste the refresh() method as it is extremely simple and we do not use anything else from the base command.
| const tableUtils: TableUtils = editor.plugins.get( 'TableUtils' ); | ||
|
|
||
| model.change( writer => { | ||
| const table = tableUtils.createTableLayout( writer, options ); |
There was a problem hiding this comment.
I'm not sure if this helper is needed.
| writer.setAttribute( 'tableWidth', '100%', table ); | ||
| writer.setAttribute( 'tableType', 'layout', table ); |
There was a problem hiding this comment.
What if the table width height attribute is unknown to the feature as it is not added to the editor? I'd remove this method.
| /** | ||
| * The table layout editing plugin. | ||
| */ | ||
| export default class TableLayoutEditing extends Plugin { |
There was a problem hiding this comment.
This should require the plugin that handles tableWidth and tableHeight. As I can see we have 2 plugins handling this attribute so maybe we should just check if that attribute is registered in the schema and warn if not?
| const { editor } = this; | ||
| const { conversion } = editor; | ||
|
|
||
| conversion.for( 'upcast' ).add( upcastTableOverride() ); |
There was a problem hiding this comment.
| conversion.for( 'upcast' ).add( upcastTableOverride() ); | |
| conversion.for( 'upcast' ).add( upcastLayoutTable() ); |
| * | ||
| * @internal | ||
| */ | ||
| function scanTable( viewTable: ViewElement ) { | ||
| export function scanTable( viewTable: ViewElement ): { headingRows: number; headingColumns: number; rows: Array<ViewElement> } { |
| export function isTableTypeContent( selectedCells: Array<Element> ): boolean { | ||
| const table = selectedCells[ 0 ].findAncestor( 'table' )!; | ||
|
|
||
| return table.getAttribute( 'tableType' ) !== 'layout'; | ||
| } |
There was a problem hiding this comment.
This looks a bit overcomplicated for a single-purpose helper. Please use other common helpers or inline attribute checks where needed.
| let isContentTable = true; | ||
|
|
||
| if ( isInTable ) { | ||
| isContentTable = isTableTypeContent( selectedCells ); | ||
| } |
There was a problem hiding this comment.
We should check if schema allows headingRows attribute instead of coupling those features.
| let isContentTable = true; | ||
|
|
||
| if ( isInTable ) { | ||
| isContentTable = isTableTypeContent( selectedCells ); | ||
| } |
| // Copy table type attribute if present. | ||
| const sourceTableType = sourceTable.getAttribute( 'tableType' ); | ||
|
|
||
| if ( sourceTableType ) { | ||
| writer.setAttribute( 'tableType', sourceTableType, croppedTable ); | ||
| } |
There was a problem hiding this comment.
This looks like a tight coupling. What should happen with tableWidth attribute?
Suggested merge commit message (convention)
Internal: Implementation of
TableLayoutplugin.Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.