Skip to content

This should fix #326.#328

Open
Mbodin wants to merge 4 commits into
ocsigen:masterfrom
Mbodin:transform
Open

This should fix #326.#328
Mbodin wants to merge 4 commits into
ocsigen:masterfrom
Mbodin:transform

Conversation

@Mbodin

@Mbodin Mbodin commented Dec 4, 2023

Copy link
Copy Markdown
Contributor

I followed the specification at https://drafts.csswg.org/css-transforms/#svg-translate as much as I could. As such, I replaced white spaces by the more general [ \t\r\f].

I tried to add some tests. I might not have placed them in the right place (I just noticed the message This file is here to torture the ppx. Tests that are directly related to html or svg should go to the other files. at the beginning of the file). Maybe some reordering should take place in the test file because of that :-\

What do you think?

@Mbodin Mbodin force-pushed the transform branch 2 times, most recently from 6f42a1c to 9a8a009 Compare December 4, 2023 16:48
@Mbodin

Mbodin commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

I'm hitting another wall there: according to the spec, it seems that skewY expects a number (degrees are assumed here I think), but according to TyXML, it expects an angle: https://github.com/ocsigen/tyxml/blob/master/lib/svg_types.mli#L302 Maybe this changed between SVG 1.1 and SVG 2 ?

@Mbodin

Mbodin commented Dec 5, 2023

Copy link
Copy Markdown
Contributor Author

I suggest to remove skew from the test and move it to another issue.

@Mbodin

Mbodin commented Dec 5, 2023

Copy link
Copy Markdown
Contributor Author

Is it an issue not to support OCaml version 4.05 or before?

@Drup

Drup commented Dec 5, 2023

Copy link
Copy Markdown
Member

This looks good! Everytime we hit similar issues, I wonder if we shouldn't have a better handling of whitespace in general, but it's pretty tricky in HTML/SVG ...

Is it an issue not to support OCaml version 4.05 or before?

No. Actually, next version will probably drop everything prior to 4.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants