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 examples for BigInt API #1380

Merged
merged 3 commits into from
Jul 10, 2019
Merged

Add examples for BigInt API #1380

merged 3 commits into from
Jul 10, 2019

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Jun 17, 2019

@Elchi3 Elchi3 requested a review from wbamberg June 17, 2019 12:38
@Elchi3 Elchi3 mentioned this pull request Jun 19, 2019
Closed
34 tasks
wbamberg
wbamberg requested changes Jul 10, 2019
View reviewed changes
Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks @Elchi3 and sorry to take so long reviewing.

live-examples/js-examples/bigint/bigint-asintn.html Outdated Show resolved Hide resolved
live-examples/js-examples/bigint/bigint-tolocalestring.html Outdated Show resolved Hide resolved
live-examples/js-examples/bigint/bigint-tostring.html Outdated Show resolved Hide resolved
live-examples/js-examples/bigint/meta.json Outdated Show resolved Hide resolved
live-examples/js-examples/bigint/meta.json Outdated Show resolved Hide resolved
@Elchi3
Copy link
Member Author

Elchi3 commented Jul 10, 2019

Thanks for the review, @wbamberg. Sorry to have poked about this one, but I thought it would be great to land it this week since we're shipping and advertising it. Addressed your comments. Great feedback as always!

wbamberg
wbamberg approved these changes Jul 10, 2019
View reviewed changes
Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks @Elchi3 , looks great. I also noticed that the "expected output" included quotes for the bigint values in asIntN() and asUintN(), which it should not.

Since it's a trivial change to remove the quotes I just made it so as to avoid another review round.

@wbamberg wbamberg merged commit 9cad363 into mdn:master Jul 10, 2019
@Elchi3 Elchi3 deleted the bigint-main-api branch July 10, 2019 14:24
@Elchi3
Copy link
Member Author

Elchi3 commented Jul 10, 2019

Thanks for the quick fix! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbamberg wbamberg wbamberg approved these changes

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
@Elchi3 @wbamberg