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.

Sign up for GitHub

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

Jump to bottom

Add <picture> tag #263

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Add <picture> tag #263

merged 2 commits into from
Mar 31, 2020

Conversation

slegrand45
Copy link
Contributor

Drup
Drup requested changes Mar 24, 2020
View reviewed changes
Copy link
Member

@Drup Drup left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks of the contribution!

I'm not sure I agree with that signature.

picture is supposed to have exactly one img child, and zero-or-more source children. The right signature is then ?a:attribs -> [< picture_content_fun] elt list_wrap -> [< img] elt -> [> picture] elt. where picture_content_fun does not contains img. There are several other tyxml elements that are implemented similarly (table, most notably).

If we go that way, we have to write a custom wrapper for the ppx.

test/test_ppx.ml Outdated
<img src=\"picture/img.png\" alt=\"test picture/img.png\" id=\"idimg\"/>
<source type=\"image/webp\" src=\"picture/img1.webp\"/>
<source type=\"image/jpeg\" src=\"picture/img2.jpg\"/>
</picture></div>"]],
Copy link
Member

Choose a reason for hiding this comment

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

Use raw string and avoid the escaped quoting.

test/test_html.ml Outdated
^ "<img src=\"picture/img.png\" alt=\"test picture/img.png\" id=\"idimg\"/>"
^ "<source type=\"image/webp\" src=\"picture/img1.webp\"/>"
^ "<source type=\"image/jpeg\" src=\"picture/img2.jpg\"/>"
^ "</picture></div>" ;
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer raw strings here ({|...|}).

lib/html_types.mli
@@ -590,7 +592,6 @@ type core_phrasing_without_interactive =
| `Wbr
| `Var
| `U
| `Img
Copy link
Member

Choose a reason for hiding this comment

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

Well spotted!

lib/html_sigs.mli
@@ -855,6 +855,9 @@ module type T = sig
alt: text wrap ->
([< img_attrib], [> img]) nullary

val picture : ([< | picture_attrib], [< | picture_content_fun], [> | picture]) star
[@@reflect.filter_whitespace]

Copy link
Member

Choose a reason for hiding this comment

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

Add link to the MDN doc ? :)

@slegrand45 slegrand45 requested a review from Drup March 24, 2020 14:17
@Drup
Copy link
Member

Drup commented Mar 31, 2020

Perfect!

@Drup Drup merged commit 4d48e60 into ocsigen:master Mar 31, 2020
@slegrand45 slegrand45 deleted the tag-picture branch March 31, 2020 11:22
Drup added a commit to Drup/opam-repository that referenced this pull request Apr 22, 2021
@Drup
[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0)
c7ce709 
CHANGES:

* Move all the PPXs to ppxlib
  (ocsigen/tyxml#271, Initial code by Sonja @pitag-ha Heinze)

* Add the `translate` attribute
  (ocsigen/tyxml#281 by Javier @jchavarri Chávarri)
* Update allowed `inputmode`s
  (ocsigen/tyxml#279 by Joel @joelburget Burget)
* Add the `picture` element
  (ocsigen/tyxml#263 by Stéphane @slegrand45 Legrand)
Drup added a commit to Drup/opam-repository that referenced this pull request Apr 22, 2021
@Drup
[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0)
8e8c295 
CHANGES:

* Move all the PPXs to ppxlib
  (ocsigen/tyxml#271, Initial code by Sonja Heinze)

* Add the `translate` attribute
  (ocsigen/tyxml#281 by Javier Chávarri)
* Update allowed `inputmode`s
  (ocsigen/tyxml#279 by Joel Burget)
* Add the `picture` element
  (ocsigen/tyxml#263 by Stéphane Legrand)
Drup added a commit to Drup/opam-repository that referenced this pull request Apr 22, 2021
@Drup
[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0)
b747b21 
CHANGES:

* Move all the PPXs to ppxlib
  (ocsigen/tyxml#271, Initial code by Sonja @pitag-ha Heinze)

* Add the `translate` attribute
  (ocsigen/tyxml#281 by Javier @jchavarri Chávarri)
* Update allowed `inputmode`s
  (ocsigen/tyxml#279 by Joel @joelburget Burget)
* Add the `picture` element
  (ocsigen/tyxml#263 by Stéphane @slegrand45 Legrand)
Drup added a commit to Drup/opam-repository that referenced this pull request Apr 22, 2021
@Drup
[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0)
ff3c4dc 
CHANGES:

* Move all the PPXs to ppxlib
  (ocsigen/tyxml#271, Initial code by Sonja @pitag-ha Heinze)

* Add the `translate` attribute
  (ocsigen/tyxml#281 by Javier Chávarri)
* Update allowed `inputmode`s
  (ocsigen/tyxml#279 by Joel Burget)
* Add the `picture` element
  (ocsigen/tyxml#263 by Stéphane Legrand)
@Drup Drup mentioned this pull request Apr 22, 2021
Merged
Drup added a commit to Drup/opam-repository that referenced this pull request Apr 22, 2021
@Drup
[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0)
dd331af 
CHANGES:

* Move all the PPXs to ppxlib
  (ocsigen/tyxml#271, Initial code by Sonja Heinze)

* Add the `translate` attribute
  (ocsigen/tyxml#281 by Javier Chávarri)
* Update allowed `inputmode`s
  (ocsigen/tyxml#279 by Joel Burget)
* Add the `picture` element
  (ocsigen/tyxml#263 by Stéphane Legrand)
Drup added a commit to Drup/opam-repository that referenced this pull request Apr 22, 2021
@Drup
[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0)
828165e 
CHANGES:

* Move all the PPXs to ppxlib
  (ocsigen/tyxml#271, Initial code by Sonja Heinze)

* Add the `translate` attribute
  (ocsigen/tyxml#281 by Javier Chávarri)
* Update allowed `inputmode`s
  (ocsigen/tyxml#279 by Joel Burget)
* Add the `picture` element
  (ocsigen/tyxml#263 by Stéphane Legrand)
Drup added a commit to Drup/opam-repository that referenced this pull request Apr 22, 2021
@Drup
[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0)
82ee2c0 
CHANGES:

* Move all the PPXs to ppxlib
  (ocsigen/tyxml#271, Initial code by Sonja Heinze)

* Add the `translate` attribute
  (ocsigen/tyxml#281 by Javier Chávarri)
* Update allowed `inputmode`s
  (ocsigen/tyxml#279 by Joel Burget)
* Add the `picture` element
  (ocsigen/tyxml#263 by Stéphane Legrand)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Drup Drup Awaiting requested review from Drup

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@slegrand45 @Drup