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

EZP-30222: Handled tables copied from external sources #106

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

konradoboza
Copy link
Member

@konradoboza konradoboza commented Jan 23, 2020

Question Answer
JIRA issue EZP-30222
Bug/Improvement yes
New feature no
Target version 1.1, master
BC breaks no
Tests pass yes
Doc needed no

This PR introduces a slightly different way of handling table width:

  • introducing normalizeWidth function which handles px and pt units as these are the main ones used in tables,
  • rounding width values, e.g. 110.5pt,
  • taking style="border: [thickness] [style] [color];" property from CSS into account and applying border="1px" to make things consistent with the tables generated by RTE by default.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@konradoboza konradoboza added Bug Something isn't working Ready for review labels Jan 23, 2020
@konradoboza konradoboza self-assigned this Jan 23, 2020
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test case for output is missing, not sure why we have them in lossy folder anyway. It feels output testing is needed here as it should be straightforward. Correct me if I'm wrong.

POV ping @dew326 can we indeed normalize pt to px as pt*1.33 = px or should we actually store units in DocBook as well?

@konradoboza
Copy link
Member Author

@alongosz that would be correct but we are actually "losing" some data here:

  • value style="border: 1px solid #000;" is converted to the default border="1" and lost as we don't handle border styling,
  • value for width inpt, as we convert and normalize it to px to not change prior behavior.

@alongosz
Copy link
Member

alongosz commented Jan 24, 2020

@alongosz that would be correct but we are actually "losing" some data here:

  • value style="border: 1px solid #000;" is converted to the default border="1" and lost as we don't handle border styling,

Ok, that makes sense. Still, can we see a test case for output (X)HTML?

  • value for width inpt, as we convert and normalize it to px to not change prior behavior.

We can indeed keep the normalization as-is right now.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal sync: output testing for lossy cases is not possible.

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

But is there coverage for:

  • Handling % unit?
  • Making sure units like em is still not stored in the backend format?

@konradoboza
Copy link
Member Author

I added some more robust handling of not supported units (others than px, pt, %) to make the whole solution a bit more bulletproof. Also, simplified rounding units as round function does casting to number by default. @andrerom @alongosz please take another look.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏆 for DRY in XSLT

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍 Indeed 🏅 for DRY here!

@konradoboza konradoboza removed their assignment Jan 30, 2020
@micszo micszo self-assigned this Jan 30, 2020
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine now!
QA Approved on eZ Platform EE v2.5.8 with diff.

@micszo micszo removed their assignment Jan 30, 2020
@lserwatka lserwatka merged commit 0f521a6 into 1.1 Jan 30, 2020
@lserwatka lserwatka deleted the EZP-30222-tables_from_ext_sources branch January 30, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
Development

Successfully merging this pull request may close these issues.

5 participants