T/9: Table feature #10
Conversation
…lspan attributes.
|
Why are |
| * @param {module:utils/eventinfo~EventInfo} eventInfo | ||
| * @param {module:engine/view/observer/domeventdata~DomEventData} domEventData | ||
| */ | ||
| _handleTabOnSelectedTable( eventInfo, domEventData ) { |
There was a problem hiding this comment.
AFAICS, Shift + Tab moves selection to the first cell as well. How about moving it to the last cell in that case? Then, you could traverse the table from the end using Shift + Tab. Looks to me like this could make some sense.
There was a problem hiding this comment.
Follow up added: https://github.com/ckeditor/ckeditor5-table/issues/21.
Probably it is not necessary - I've removed them and it looks like it works OK without |
src/converters/downcast.js
Outdated
| const tableWalker = new TableWalker( table ); | ||
|
|
||
| const tableAttributes = { | ||
| headingRows: parseInt( table.getAttribute( 'headingRows' ) || 0 ), |
There was a problem hiding this comment.
Is parseInt really needed here? Aren't attributes in model already numbers (or undefined)?
There was a problem hiding this comment.
Weren't there some problems with strings? I don't recall how it was working but I've managed to have colspan or other attributes as: "11111" in some places.
There was a problem hiding this comment.
That was probably in upcast. In DOM (and then probably also in the view) attributes are strings. Amirite?
In the model, attributes stay as set. You can even add objects as attributes.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm using a default converters here so the values are strings.
There was a problem hiding this comment.
Actually I've fixed this for headingRows & headingColumns - so table attributes. The colspan and rowspan parsing must use parseInt() for now.
src/converters/downcast.js
Outdated
| headingColumns: parseInt( table.getAttribute( 'headingColumns' ) || 0 ) | ||
| }; | ||
|
|
||
| // We need to iterate over a table in order to get proper row & column values from a walker |
There was a problem hiding this comment.
Can't we start iterating from the proper row already (get the row index straight from the model: data.item.parent.index)? Isn't it that rows are set even if there are rowspans?
There was a problem hiding this comment.
Yep, adding startRow & endRow will do the trick but anyway the tableWalkerValue is needed to properly calculate if this table cell is in heading section so the tableWalker is still required.
src/converters/upcasttable.js
Outdated
| } | ||
|
|
||
| // Upcast table rows in proper order (heading rows first). | ||
| rows.forEach( row => conversionApi.convertItem( row, ModelPosition.createAt( table, 'end' ) ) ); |
There was a problem hiding this comment.
I know that this will not do anything if rows.length == 0 but I'd put it into else for clarity reasons.
Suggested merge commit message (convention)
Feature: Initial table support. Closes ckeditor/ckeditor5#3162, Closes ckeditor/ckeditor5#3165, Closes ckeditor/ckeditor5#3166.
Additional information
Based on @Reinmar's input I've updated the code from #6 and this PR introduces:
colspanandrowspanattributes handling.What this PR does not cover from MVP (ckeditor/ckeditor5#3163):
<p>inside table cell thingyI think that there might be some small issues with commands - resulting in broken table in some situations (mostly related to
rowspan/colspan) but those might be fixed after we start doing UI since using commands from console is not ideal.