Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

I/6179: Fix alignment property converter #233

Closed
wants to merge 4 commits into from
Closed

I/6179: Fix alignment property converter #233

wants to merge 4 commits into from

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Feb 5, 2020

Suggested merge commit message (convention)

Fix: Table's alignment attribute converter should override Alignement feature converter. Closes ckeditor/ckeditor5#6179.


Additional information

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2b3511c on i/6179 into be0d759 on master.

@jodator
Copy link
Contributor Author

jodator commented Feb 5, 2020

There are some changes in the Chrome 80 in comparison to Chrome 79... I've just updated to Chrome 80 and indeed this one test fail. Was green on Chrome 79.

@Reinmar
Copy link
Member

Reinmar commented Feb 6, 2020

👍 for consuming the attribute – that's definitely a good change.

👎 for changing the priority of the converter. As I commented in ckeditor/ckeditor5#6179 (comment) the alignment feature converter should be smarter. Semantically, the alignment feature applies to text blocks (it sets text-alignment so it affects the content of those elements). So, it should not apply to object elements.

In other words – IMO, instead of rising the priority of the table alignment converter we should make the alignment feature's converter apply only to non-object elements.

@jodator
Copy link
Contributor Author

jodator commented Feb 6, 2020

-1 for changing the priority of the converter. As I commented in ckeditor/ckeditor5#6179 (comment) the alignment feature converter should be smarter. Semantically, the alignment feature applies to text blocks (it sets text-alignment so it affects the content of those elements). So, it should not apply to object elements.

In other words – IMO, instead of rising the priority of the table alignment converter we should make the alignment feature's converter apply only to non-object elements.

As in F2F conversation: re-writing the Alignment feature converter is not a half-hour task so we extracted this to: ckeditor/ckeditor5#6199.

@Reinmar other than that this PR is ready to review (and merge - the only failing test fails on master also because of failing tests on Chrome 80.

@jodator
Copy link
Contributor Author

jodator commented Feb 6, 2020

Invalidated by #234 - the tests remained untouched though.

@jodator jodator closed this Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table alignment property is not working properly
3 participants