Rewrite JsxDOM.ml to res #5723

mununki · 2022-10-04T02:00:00Z

I know that JsxDOM needs to be rewritten anyway when the new record extension is landed. But this PR would improve such things from current.

aria-* attribute with proper polymorphic variant expression e.g. #"true" instead #_true
OCaml reserved keyword begin_, end_, to_ are deprecated, now begin, end, to are available

jscomp/others/release.ninja

mununki · 2022-10-04T04:29:17Z

I guess I should have run make artifacts?

cknitt · 2022-10-04T05:13:07Z

jscomp/others/jsxDOM.res

@@ -0,0 +1,621 @@
+/* Copyright (C) 2015-2016 Bloomberg Finance L.P.


Bloomberg Finance does not seem right to me.

@cristianoc What should the copyright header be like for new files?

Maybe

Copyright (C) 2022- Authors of ReScript

or something like that?

That looks good

Ok, then we should probably also add this to the CONTRIBUTING doc, too.

updated 23f5fe0

cknitt

There are quite a few properties that are strings now and could actually be enums (I listed a few of them below). But maybe we do that in a separate cleanup?

cknitt · 2022-10-04T05:15:06Z

jscomp/others/jsxDOM.res

+  @as("aria-autocomplete") ariaAutocomplete?: [#inline | #list | #both | #none],
+  /* https://www.w3.org/TR/wai-aria-1.1/#valuetype_tristate */
+  @as("aria-checked")
+  ariaChecked?: [


Unified formatting across this file would be nice. I.e. have this on one line, and run the formatter over the file.

Oh I ran the formatter too, in my case it added a line-break 🤷‍♂️

I'll make it to one line, it looks better.

jscomp/others/jsxDOM.res

mununki · 2022-10-04T12:20:41Z

@cknitt I've changed most of string types you mentioned to enum. I've referred to mdn docs.

cknitt · 2022-10-04T12:22:39Z

Thanks! There will be more props that are actually enums though, e.g. underlinePosition.

Maybe we should do the enum conversion separately and make sure it is complete?

mununki · 2022-10-04T12:25:30Z

Thanks! There will be more props that are actually enums though, e.g. underlinePosition.

Maybe we should do the enum conversion separately and make sure it is complete?

Sounds good to me.
Not sure underlinePosition can be a enum? Is it https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/underline-position?

cknitt · 2022-10-04T12:30:36Z

Ah, right, sorry, confused it with https://developer.mozilla.org/en-US/docs/Web/CSS/text-underline-position.

mununki · 2022-10-04T12:41:44Z

Ah, right, sorry, confused it with https://developer.mozilla.org/en-US/docs/Web/CSS/text-underline-position.

Ah! text-underline-position seems CSS property though. 😃

cknitt · 2022-10-04T12:58:03Z

Yes, googled it wrong. 😉

Still, I feel we should do the string -> enum conversions in a separate PR, possibly together with #5713.
@cristianoc what do you think?

cristianoc · 2022-10-04T12:59:46Z

Is this for existing props or new props not exposed before?
If existing, enums are a breaking change right.

cristianoc · 2022-10-04T13:00:59Z

So yes I guess separate PR.

mununki · 2022-10-04T13:30:48Z

Is this for existing props or new props not exposed before? If existing, enums are a breaking change right.

Oops, you're right. I think 23f5fe0 should be reverted, too many breaking I guess. What do you think @cknitt ?
Revert it and separated PR would be better.

cknitt · 2022-10-04T13:34:35Z

23f5fe0 is the copyright only?
a2021bd should be reverted I think.

mununki · 2022-10-04T13:35:23Z

a2021bd

Oops, confused my turn 😉 Yes, a2021bd is correct.

mununki · 2022-10-04T14:10:20Z

@cknitt I reverted it 1f63c87 except ariaChecked which is not existed one.

mununki · 2022-10-04T16:26:09Z

@cknitt I mistakenly dropped the children field from JsxDOM.ml, I added it back.

mununki · 2022-10-05T00:00:29Z

@cknitt I think I'm done with this PR. Could you check another round?

cristianoc · 2022-10-05T05:42:51Z

This looks good to me. Would you mention the deprecated fields in the CHANGELOG?

cknitt

@mattdamon108 Looks good to me! 👍

mununki · 2022-10-05T14:35:00Z

This looks good to me. Would you mention the deprecated fields in the CHANGELOG?

20d6003

cristianoc

Looks ready to go

mununki added 3 commits October 4, 2022 09:43

rewrite jsxDOM.ml to res

352c6c0

remove jsxDOM.ml

2eb8aa6

snapshot

7ae764c

mununki requested a review from cknitt October 4, 2022 02:00

mununki commented Oct 4, 2022

View reviewed changes

jscomp/others/release.ninja Outdated Show resolved Hide resolved

Done make artifacts

45a3005

mununki mentioned this pull request Oct 4, 2022

Convert JsxDOM.ml to JsxDOM.res and add missing aria attributes #5708

Closed

cknitt reviewed Oct 4, 2022

View reviewed changes

mununki added 4 commits October 4, 2022 21:04

fix missing optional

378e1c1

change copyright

23f5fe0

change to enum and clean up

a2021bd

snapshot

0e2102a

mununki requested a review from cknitt October 4, 2022 12:19

mununki added 2 commits October 4, 2022 23:06

revert enum to avoid breaking

1f63c87

snapshot

95133a2

cknitt approved these changes Oct 4, 2022

View reviewed changes

mununki added 2 commits October 5, 2022 01:24

add missing children field back

446a412

snapshot

69473eb

cknitt approved these changes Oct 5, 2022

View reviewed changes

update changelog

20d6003

cristianoc approved these changes Oct 5, 2022

View reviewed changes

mununki merged commit f8d1d3a into 10.1_release Oct 5, 2022

mununki deleted the rewrite-jsxdom branch October 5, 2022 23:00

cknitt mentioned this pull request Oct 7, 2022

How to use aria-current? rescript-lang/rescript-react#28

Closed

		@@ -0,0 +1,621 @@
		/* Copyright (C) 2015-2016 Bloomberg Finance L.P.

Rewrite JsxDOM.ml to res #5723

Rewrite JsxDOM.ml to res #5723

Uh oh!

Conversation

mununki commented Oct 4, 2022

Uh oh!

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

cknitt Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

cknitt Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

mununki Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

cknitt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cknitt Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

mununki Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

mununki Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

cknitt commented Oct 4, 2022

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

cknitt commented Oct 4, 2022

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

cknitt commented Oct 4, 2022

Uh oh!

cristianoc commented Oct 4, 2022

Uh oh!

cristianoc commented Oct 4, 2022

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

cknitt commented Oct 4, 2022

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

mununki commented Oct 4, 2022

Uh oh!

mununki commented Oct 5, 2022

Uh oh!

cristianoc commented Oct 5, 2022

Uh oh!

cknitt left a comment

Choose a reason for hiding this comment

Uh oh!

mununki commented Oct 5, 2022

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

cknitt left a comment •

edited

Loading