Skip to content

Review fixes for #44. #46

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

Merged
merged 28 commits into from
Sep 16, 2015
Merged

Review fixes for #44. #46

merged 28 commits into from
Sep 16, 2015

Conversation

zolkis
Copy link
Contributor

@zolkis zolkis commented Sep 7, 2015

Fixing comments for #44.

@zolkis
Copy link
Contributor Author

zolkis commented Sep 7, 2015

Thanks, see commit 3 in this PR.

@@ -1790,19 +1793,19 @@
When an <a>NFC device</a> comes in communication range,
<ul>
<li>
If <var>options.target</var> has the value <code>"tag"</code>,
If <var>target</var> has the value <code>"tag"</code>,
Copy link
Member

Choose a reason for hiding this comment

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

If the options object's target member has the value "tag",

Copy link
Member

Choose a reason for hiding this comment

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

@zolkis is right for the current version of the document, since step 9 introduces a local variable named target.

If it weren't a local variable, I've been using the convention <code><var>options</var>.target</code> to pull out members of dictionaries.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I stand corrected. The algo is rather long to parse on a small screen.

@anssiko
Copy link
Member

anssiko commented Sep 8, 2015

Thanks for the PR, I added some inline comments.

@zolkis
Copy link
Contributor Author

zolkis commented Sep 8, 2015

Thanks Anssi, I will fix them in later commits, don't merge until fixed. By now I will have to include them with other changes.

@zolkis zolkis changed the title Small fixes: parameters handling, editorials. Review fixes for #44. Sep 8, 2015
@@ -1418,6 +1461,9 @@
</ul>
</li>
<li>
Let <var>options</var> be the second argument.
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this, since you've introduced it with pushMessage(message, options) above.

Copy link
Member

Choose a reason for hiding this comment

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

The more verbose style is consistent with e.g. the HTML spec. I suggest we prefer clarity over brevity. But I don't feel strongly, so leaving it to the editors.

@anssiko
Copy link
Member

anssiko commented Sep 15, 2015

@jyasskin The editors believe your feedback has now been addressed. Could you please review this PR, and let @zolkis and @kenchris know if you'd like the editors to do further changes before merging. This version of the spec does not need to be perfect, but it should be a good base for the next iteration of the experimental implementation.

For convenience, here's the RawGit HTML preview of the head of this PR.

Thank you again for your extensive and detailed feedback.

@anssiko
Copy link
Member

anssiko commented Sep 15, 2015

Since GH has issues showing the changes, you can use the HTML diff to ease the review.

@anssiko
Copy link
Member

anssiko commented Sep 16, 2015

Will merge this PR now and suggest the editors address the remaining feedback in another PR.

anssiko added a commit that referenced this pull request Sep 16, 2015
@anssiko anssiko merged commit 00b86e7 into w3c:gh-pages Sep 16, 2015
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.

4 participants