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

Update dom/form data #800

Merged
merged 10 commits into from
Aug 1, 2023
Merged

Update dom/form data #800

merged 10 commits into from
Aug 1, 2023

Conversation

G-yhlee
Copy link
Contributor

@G-yhlee G-yhlee commented Jul 26, 2023

related issue #798

@zetashift
Copy link
Contributor

@G-yhlee I messed up my git-fu, but if you can accept this PR for MDN docstrings on your fork G-yhlee#1 then I believe we're good to go!

Add MDN documentation strings for FormData
Copy link
Contributor

@zetashift zetashift left a comment

Choose a reason for hiding this comment

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

Thank you!!

* The field's value. This can be a string or `Blob` (including subclasses such as File). If none of these are
* specified the value is converted to a string.
*/
def append(name: js.Any, value: String): Unit = js.native
Copy link
Member

Choose a reason for hiding this comment

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

Why is name: js.Any, isn't only String allowed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, the problem was caused by not fixing the original append method properly.
name must be a string. I just updated it, thank you !

@G-yhlee G-yhlee requested a review from armanbilge July 27, 2023 16:05
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Sorry, I noticed just a few more things. Thanks for your work on this!


/** XMLHttpRequest Level 2 adds support for the new FormData interface. FormData objects provide a way to easily
* construct a set of key/value pairs representing form fields and their values, which can then be easily sent using
* the XMLHttpRequest send() method.
*/
@js.native
@JSGlobal
class FormData(form: HTMLFormElement = js.native) extends js.Object {
class FormData(form: HTMLFormElement = js.native) extends js.Iterable[js.Tuple2[String, String]] {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class FormData(form: HTMLFormElement = js.native) extends js.Iterable[js.Tuple2[String, String]] {
class FormData(form: HTMLFormElement = js.native) extends js.Iterable[js.Tuple2[String, String | Blob]] {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clear

dom/src/main/scala/org/scalajs/dom/FormData.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/FormData.scala Outdated Show resolved Hide resolved

/** XMLHttpRequest Level 2 adds support for the new FormData interface. FormData objects provide a way to easily
* construct a set of key/value pairs representing form fields and their values, which can then be easily sent using
* the XMLHttpRequest send() method.
*/
@js.native
@JSGlobal
class FormData(form: HTMLFormElement = js.native) extends js.Object {
class FormData(form: HTMLFormElement = js.native) extends js.Iterable[js.Tuple2[String, String]] {
Copy link
Member

@armanbilge armanbilge Aug 1, 2023

Choose a reason for hiding this comment

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

Also, it seems there are three possible constructors.

new FormData()
new FormData(form)
new FormData(form, submitter)
class FormData extends ... {
  def this(form: HTMLFormElement) = this()
  def this(form: HTMLFormElement, submitter: HTMLElement) = this()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class FormData(form: HTMLFormElement = js.native) extends ... {
  def this(form: HTMLFormElement) = this(form)
  def this(form: HTMLFormElement, submitter: HTMLElement) = this(form)
}

I think this is good point !
I've looked at other api codes using this() and i make this code,
but I'm not sure if this change is correct.

I'd be grateful if someone could fix this part for sure

Copy link
Member

Choose a reason for hiding this comment

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

I've looked at other api codes using this() and i make this code,

That won't work, because there are two constructors that take a form. How wil it know which one to use?

I believe the correct change is the one I suggested in #800 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it !
I will add this part :)

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for the revisions. Nice work on this!!

@armanbilge armanbilge merged commit 8c52373 into scala-js:main Aug 1, 2023
5 checks passed
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.

3 participants