Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimal table support #3162

Closed
jodator opened this issue Feb 22, 2018 · 11 comments · Fixed by ckeditor/ckeditor5-table#10
Closed

Minimal table support #3162

jodator opened this issue Feb 22, 2018 · 11 comments · Fixed by ckeditor/ckeditor5-table#10
Labels
package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@jodator
Copy link
Contributor

jodator commented Feb 22, 2018

I'd like to propose incremental work on table feature as it will take some time and effort to fully develop even in MVP form.

As the minimal table support I'd like to propose:

  • Table conversion (basic upcast/downcast that will not destroy existing data) with:
    • tbody/thead/tfoot parsing,
    • tr,td,th handling,
    • attributes parsing (at least colspan & rowspan).
  • Insert table button (probably temporary) that will insert 2x2 table.
  • Insert table command (maybe with parameters already like rows & columns).

And basically that. No cursor support, no better UI, no widget and probably buggy in one way or another but I think that setting small steps along the way might be useful and probably better to review during that time rather one big PR.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

LGTM.

And how will headers be represented in the model?

@jodator
Copy link
Contributor Author

jodator commented Feb 22, 2018

As for now the row has attribute set:

https://github.com/ckeditor/ckeditor5-table/blob/9f245d0eab2b7813845d355ac78796405815c9bb/src/converters.js#L96-L103
and rows are wrapped in its section

https://github.com/ckeditor/ckeditor5-table/blob/9f245d0eab2b7813845d355ac78796405815c9bb/src/converters.js#L70-L80

I've just fiddled a bit with conversion and how this might be represented so that wasn't overly thought out.

Other option is type attribute with one of 'foot|body|head' value.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

Can we represent this on the <table> element? This way, we'll avoid situations when e.g. first row is a heading, the second one isn't, and the 3rd one is a heading again.

Also:

  • please remember that we'll need to support situations where e.g. first two rows are headings,
  • please don't implement support for <tfoot> yet because we shouldn't support markup that we don't allow to edit because this may create odd situations when someone pasted such content but can't do anything with it,
  • please remember that not only rows can be headers but also columns.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

I'm wondering – what about a situation when you'd like to have a heading row in the middle of the table. E.g. the table may be super long and you might want to repeat the heading in the middle of its length (so to make the table more readable).

That wouldn't work with our UI and the model where only the first N rows can be headers.

However, I'd say that this would be a specific feature of the editor which automatically repeats the table head every N rows. Does it make sense? Or should be "loosen" this feature to allow creating headings wherever you want?

cc @oleq @Comandeer

@jodator
Copy link
Contributor Author

jodator commented Feb 22, 2018

Can we represent this on the <table> element? This way, we'll avoid situations when e.g. first row is a heading, the second one isn't, and the 3rd one is a heading again.

They are group together - but yes I can move that to table and remove tfoot parsing.

The other thing is how to deal with more then one thead in the markup 🤔 we might either merge them or ignore anything above the first one.

As for column headers - as for now I'm supporting this on tableCell level since you can have td in row heading section.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

PS. https://stackoverflow.com/questions/5395228/html-tables-thead-vs-th:

"This division enables user agents to support scrolling of table bodies independently of the table head and foot. When long tables are printed, the table head and foot information may be repeated on each page that contains table data."

So my point about repeating rows was highly invalid. Although, are any browsers actually implementing this feature? :D

@jodator
Copy link
Contributor Author

jodator commented Feb 22, 2018

I was thinking about such case:
https://github.com/ckeditor/ckeditor5-table/blob/9f245d0eab2b7813845d355ac78796405815c9bb/tests/manual/tables.html#L38-L50

but it is probably too detailed to suport.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

The other thing is how to deal with more then one thead in the markup 🤔 we might either merge them or ignore anything above the first one.

I think that:

  • This is not a common mistake, so initially we could do nothing. We strip them anyway, so after being passed through the model we'll have clean output.
  • In the future we can simply mark all rows which were in <thead> as headings (they will be first rows of a table, so we'd set table[headingRows=N] where N = sum of all rows which were in <thead>.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

As for column headers - as for now I'm supporting this on tableCell level since you can have td in row heading section.

It'd be good to unify this with heading rows. Sth like this:

<table headingRows=1 headingCols=1>
    <tr>
        <td>heading</td>
        <td>heading</td>
        <td>heading</td>
    </tr>
    <tr>
        <td>heading</td>
        <td>not a heading</td>
        <td>not a heading</td>
    </tr>
    <tr>
        <td>heading</td>
        <td>not a heading</td>
        <td>not a heading</td>
    </tr>

cc @pjasiun did you imagine something like this?

@jodator
Copy link
Contributor Author

jodator commented Feb 22, 2018

Yeah - if we just say that we support most common cases then yes: attribute on the table is enough.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

Yeah - if we just say that we support most common cases then yes: attribute on the table is enough.

I think that in the future, if we'll ever need to mark specific cells as headings, we'll simply mark these specific cells using td's attributes. It makes a semantical sense – a table can have some rows/cols as headings + some customization is possible which would override table's settings. And we can roll this out iteratively, when needed.

PS. CKEditor 4 also allows setting headings in the table cell props and in each cell's props dialogs.

Reinmar referenced this issue in ckeditor/ckeditor5-table May 29, 2018
Feature: Initial table support. Closes #4. Closes #7. Closes #9.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-table Oct 9, 2019
@mlewand mlewand added this to the iteration 18 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:table labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
3 participants