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

Add svg fill-rule attribute #294

Merged
merged 3 commits into from
Jun 21, 2022
Merged

Conversation

dedbox
Copy link
Contributor

@dedbox dedbox commented Dec 28, 2021

Hello!

I noticed that there is a `Fill_rule constructor in Svg_types.presentation_attr, but I could not find an attribute function that uses it. So, this PR adds an a_fill_rule attribute function that accepts `Nonzero or `Evenodd.

See https://www.w3.org/TR/SVG11/painting.html#FillRuleProperty for details on the fill-rule attribute.

I believe this branch is at least mostly ready for review. Since this is my first contribution, I'm not entirely sure. I have succeeded in using a_fill_rule from my local fork in another project, and the existing unit tests are passing, but I have not added any new unit tests.

@dedbox dedbox marked this pull request as ready for review December 28, 2021 04:51
@dedbox dedbox changed the title add SVG fill-rule attribute Add svg fill-rule attribute Dec 28, 2021
@Drup
Copy link
Member

Drup commented Dec 30, 2021

Looks great, thanks !

Since it has a new type and a new ppx rule, please add a least a small test there, and then I'll merge.

@dedbox
Copy link
Contributor Author

dedbox commented Dec 31, 2021

Nice! Since there are only two valid inputs to a_fill_rule, I just added checks for both to each of the jsx and ppx test suites.

@smorimoto smorimoto requested a review from Drup January 2, 2022 16:15
@Drup
Copy link
Member

Drup commented Jun 21, 2022

Thanks @dedbox ! :)

@Drup Drup merged commit b379b73 into ocsigen:master Jun 21, 2022
Drup added a commit to Drup/opam-repository that referenced this pull request Sep 29, 2023
CHANGES:

* Update for OCaml 5.0 and drop support for OCaml 4.2.0
  (ocsigen/tyxml#312 by @rr0gi)

* Add additional variants to `linktype` for the `rel` attribute
  (Leon @LogicalOverflow Vack)

* Expand options for `autocomplete` attribute on `<input>` elements
  (ocsigen/tyxml#302 by Aron @aronerben Erben)

* Fix the SVG element `<animate>` (by the way, deprecate `animation` et
  al. in favor of `animate` et al.)
  (ocsigen/tyxml#306 by Idir @ilankri Lankri)

* Add support for `dialog` element and `onclose` attribute
  (ocsigen/tyxml#301 by Julien Sagot)
* Add an escape hatch for emitting attributes with non-standard names
  in jsx or ppx code (a leading `_` character on attribute name)
  (ocsigen/tyxml#295 Chas @cemerick Emerick)
* Add support for `type` attribute on `<script>` elements
  (ocsigen/tyxml#293 by Ulrik @ulrikstrid Strid and Chas @cemerick Emerick)

* Add svg `fill-rule` attribute
  (ocsigen/tyxml#294 by Eric @dedbox Griffis)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

* Update for OCaml 5.0 and drop support for OCaml 4.2.0
  (ocsigen/tyxml#312 by @rr0gi)

* Add additional variants to `linktype` for the `rel` attribute
  (Leon @LogicalOverflow Vack)

* Expand options for `autocomplete` attribute on `<input>` elements
  (ocsigen/tyxml#302 by Aron @aronerben Erben)

* Fix the SVG element `<animate>` (by the way, deprecate `animation` et
  al. in favor of `animate` et al.)
  (ocsigen/tyxml#306 by Idir @ilankri Lankri)

* Add support for `dialog` element and `onclose` attribute
  (ocsigen/tyxml#301 by Julien Sagot)
* Add an escape hatch for emitting attributes with non-standard names
  in jsx or ppx code (a leading `_` character on attribute name)
  (ocsigen/tyxml#295 Chas @cemerick Emerick)
* Add support for `type` attribute on `<script>` elements
  (ocsigen/tyxml#293 by Ulrik @ulrikstrid Strid and Chas @cemerick Emerick)

* Add svg `fill-rule` attribute
  (ocsigen/tyxml#294 by Eric @dedbox Griffis)
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