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 support for Web NFC API #806

Merged
merged 3 commits into from
Dec 21, 2023
Merged

Add support for Web NFC API #806

merged 3 commits into from
Dec 21, 2023

Conversation

FabioPinheiro
Copy link
Contributor

@FabioPinheiro

This comment was marked as outdated.

Sign in to view
@FabioPinheiro

This comment was marked as resolved.

Sign in to view
@FabioPinheiro FabioPinheiro changed the title [WIP] Add support for Web NFC API Add support for Web NFC API Aug 12, 2023
FabioPinheiro added a commit to FabioPinheiro/scala-did that referenced this pull request Aug 16, 2023
@FabioPinheiro
scala-js/scala-js-dom#806: Add support for Web NFC API
89966dc
armanbilge
armanbilge reviewed Aug 21, 2023
View reviewed changes
Copy link
Member

@armanbilge armanbilge 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.

Sorry, I've had some review comments "pending" for a week. I haven't finished looking at all the changes carefully, but here are my comments so far. Thanks for your work and your patience!

dom/src/main/scala/org/scalajs/dom/NDEFMessage.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFReader.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFReader.scala Outdated Show resolved Hide resolved
@FabioPinheiro FabioPinheiro force-pushed the Web_NFC_API branch 2 times, most recently from 0742064 to d6dce7c Compare August 22, 2023 00:17
FabioPinheiro added a commit to FabioPinheiro/scala-did that referenced this pull request Aug 22, 2023
@FabioPinheiro
After update PR scala-js/scala-js-dom#806
a753c1a
FabioPinheiro added a commit to FabioPinheiro/scala-did that referenced this pull request Aug 24, 2023
@FabioPinheiro
Reflex change on scala-js/scala-js-dom#806
07359b1
@FabioPinheiro FabioPinheiro mentioned this pull request Sep 28, 2023
Merged
@FabioPinheiro
Copy link
Contributor Author

@armanbilge Thanks for the review
I only saw it today when I was updating the version of the lib.
If you could review it again would be much appreciated I have a couple of questions

armanbilge
armanbilge reviewed Sep 29, 2023
View reviewed changes
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.

I'm sorry, the example I linked was wrong 😕 all these defs should be vars.

dom/src/main/scala/org/scalajs/dom/NDEFWriteOptions.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFScanOptions.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecordInit.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecordInit.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecordInit.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecordInit.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecordInit.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecordInit.scala Outdated Show resolved Hide resolved
@FabioPinheiro
Add support for Web NFC API
bd14f33 
Apply suggestions from code review
Co-authored-by: zetashift <[email protected]>
Co-authored-by: Arman Bilge <[email protected]>
armanbilge
armanbilge reviewed Sep 29, 2023
View reviewed changes
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.

Hmm, did you change those defs to vars? Some of them should be defs. If it is "read-only", then it is a def.

dom/src/main/scala/org/scalajs/dom/NDEFMessage.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFReadingEvent.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFReadingEvent.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFReadingEvent.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecord.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecord.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecord.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecord.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecord.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/NDEFRecord.scala Outdated Show resolved Hide resolved
@FabioPinheiro
Copy link
Contributor Author

https://w3c.github.io/web-nfc/ format seems more clear to follow.
I did another review of all fields to seem if they are optional or not.
Hopefully everything is correct now.

FabioPinheiro added a commit to FabioPinheiro/scala-did that referenced this pull request Sep 29, 2023
@FabioPinheiro
Reflect changes in scala-js/scala-js-dom#806 after review
07fd6ce
FabioPinheiro added a commit to FabioPinheiro/scala-did that referenced this pull request Sep 29, 2023
@FabioPinheiro
Reflect changes in scala-js/scala-js-dom#806
b3f5fc8
zetashift
zetashift approved these changes Oct 14, 2023
View reviewed changes
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.

LGTM! Thank you for this awesome PR!

@FabioPinheiro
Copy link
Contributor Author

Would be nice to have this merged before the next release

@armanbilge armanbilge self-requested a review November 26, 2023 17:16
@zetashift zetashift merged commit 6c9e6b6 into scala-js:main Dec 21, 2023
5 checks passed
@zetashift
Copy link
Contributor

@FabioPinheiro sorry for the wait, and thank you so much for this PR!

@zetashift zetashift mentioned this pull request Dec 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zetashift zetashift zetashift approved these changes

@armanbilge armanbilge Awaiting requested review from armanbilge

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

Successfully merging this pull request may close these issues.
3 participants
@FabioPinheiro @zetashift @armanbilge