I/6197: Proper convertion of the table alignment property.

[jodator]

Suggested merge commit message (convention)

Other: Convert alignment table property to a <figure> element. Closes ckeditor/ckeditor5#6197. Closes ckeditor/ckeditor5#6179.

---

Additional information

@Reinmar This PR invalidates fixes in #233. Fortunately, the test written there are helpful so the time didn't get wasted.

Some question though

1. I've left conversion to a style not a class like in Image plugins - are we OK with this.
2. How to convert center (up/down) - I've left the old one margin-right:auto;margin-left:auto but we should be OK with empty style. In this - should we ever have center property in the model? Looks superfluous and also was raised by @oleq (but I can't find where)

[oleq]

2. Looks superfluous and also was raised by @oleq (but I can't find where)

Here.

[oleq]

Something's not right (🥁) here

[oleq]

The conversion is mirrored.

[jodator]

The conversion is mirrored.

mirrored and fixed.

[coveralls]

Coverage remained the same at 100.0% when pulling ea06fc7 on i/6197 into 37f3796 on master.

[oleq]

I've left conversion to a style not a class like in Image plugins - are we OK with this.

How to convert center (up/down) - I've left the old one margin-right:auto;margin-left:auto but we should be OK with empty style. In this - should we ever have center property in the model? Looks superfluous and also was raised by @oleq (but I can't find where)

As for the first question, my gut tells me unlike inline styles, classes have the future (.table-style-right). They give developers some options like they can alter the look of aligned tables or address some issues using .table.table-style-right + .something-that-follows styling.

The second is a hard one.

• Something tells me we should've implemented align "unset" button in the UI.
• This is strongly related to default table content styles.
  • On one hand, when table props is not loaded, all tables should be centered by default content styles.
  • On the other hand, when the feature is loaded, those styles should no longer apply because each table is aligned declaratively by the user.
    • ...and probably by default, a new table inserted into the content with the table props loaded should be aligned to the left
      • But then... we have "align left" button in the UI, which is not related to the default look but to the particular attribute value in the model, so default left ≠ left in the UI. Here the "unset" button could help.
  • All this causes issues with docs. On the same page there will be editors with and without table props.
    • The former should not be affected by the content styles of the latter, because each table is styled individually.
    • The later should have this default centring of tables in the content.
    • This is the only idea that popped into my mind how to solve this in CSS table:not(.table-style-right):not(.table-style-left):not(.table-style-center) { /* the default centring */ }

@Reinmar Can we know your opinion?

[jodator]

• by default, a new table inserted into the content with the table props loaded should be aligned to the left

I'm for a new table to be centered as in the case when the feature is not loaded.

[Reinmar]

This features (table styles) is about inline styles, so inline styles are the way to go.

The image-like table styles is a separate ticket: ckeditor/ckeditor5#3225. The problem that I realised right now is the naming conflict ;| I'm afraid that we have to rename the feature that we're building right now to something like "table formatting" in order to allow "table styles" in the future. WDYT? If you're fine, let's report a ticket :(

As for what options we should have right now, that'd be my idea:

• Lack of style == the middle option (centred table) highlighted and the table should indeed be centred by our default styles – with and without the table formatting feature.
• The other two options – left and right aligned will use float:left and float:right and it seems straighforward.

[Reinmar]

The image-like table styles is a separate ticket: ckeditor/ckeditor5#3225. The problem that I realised right now is the naming conflict ;| I'm afraid that we have to rename the feature that we're building right now to something like "table formatting" in order to allow "table styles" in the future. WDYT? If you're fine, let's report a ticket :(

Or rather "TableProperties"?

[Reinmar]

Or rather "TableProperties"?

OK... it's how it's called currently :D Thank god :D

[oleq]

Is it normal that a centered table has the style attribute in view equal to ;?

It should be gone IMO.

[oleq]

Also, why is there text-align:center (<figure class="table" style="text-align:center;">) in the editor data output for a centered table? It does not affect the <table>.

[jodator]

Is it normal that a centered table has the style attribute in view equal to ;?

Bug: ckeditor/ckeditor5#6225.

[jodator]

@oleq - the issue with center alignment was fixed. The problem was integration with the alignment. It picked up "center" which was not converted by TableProperties. It was however bug in the UI since we agreed on '' to be a default value (centered visually). Making UI to unset value for the "center" button and mapping '' to a isOn of the center alignment button did the trick.

[Reinmar]

Unfortunately we have a couple conflicts on this branch. Could you resolve them, @jodator?

[Reinmar]

As commented.

[Reinmar]

Is it still necessary?

[jodator]

yes: ckeditor/ckeditor5#6199

[Reinmar]

I left two comments. Probably those things can be removed.

[jodator]

@Reinmar / @oleq I've merged this one as it blocks me on ckeditor/ckeditor5#6232 and I feel that most of the comments were addressed.