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

move "set a configuration" algorithm #1233

Merged
merged 6 commits into from
Jul 6, 2017
Merged

Conversation

fippo
Copy link
Contributor

@fippo fippo commented May 22, 2017

moves the algorithm to set a configuration to the operations section.
Also mentions the [[configuration] internal slot in the constructor.

fixes #1200


Preview | Diff

@fluffy
Copy link
Contributor

fluffy commented May 29, 2017

This is very hard to review. I'd prefer to do this as two separate PRs - one that just moves the text with no changes to it, and a second that makes any changes.

@fippo
Copy link
Contributor Author

fippo commented May 29, 2017

i'll try to separate the commits again

@stefhak
Copy link
Contributor

stefhak commented Jun 15, 2017

@fippo can you separate commits?

@fippo
Copy link
Contributor Author

fippo commented Jun 20, 2017

done and squashed/rebased. Old branch/commit is retained here .

Sorry for taking so long, had to make sure iprbot still 💕 me.

@taylor-b
Copy link
Contributor

taylor-b commented Jun 22, 2017

@fippo Sorry to be picky, but can you separate this into separate PRs as well, so it's easier to review the diff?

@fippo
Copy link
Contributor Author

fippo commented Jun 22, 2017

@taylor-b try these diffs:
786ebaf
8fbb83e
9228c02
14ff3e2

separate PRs cant be squashed :-/

@taylor-b
Copy link
Contributor

taylor-b commented Jun 29, 2017

Aside from the comment above, and the fact that there are merge conflicts now (sorry), this LGTM

@fippo
Copy link
Contributor Author

fippo commented Jun 29, 2017

hold on, there still seem to be merge conflicts...

@fippo
Copy link
Contributor Author

fippo commented Jun 29, 2017

rebased, kept the same commit structure, fixed the throw in the respec commit and added another commit that makes the internal slot consistent with #1360

webrtc.html Outdated
@@ -1022,7 +1022,8 @@
<p>Initialize <var>connection</var>'s <a>ICE Agent</a>.</p>
</li>
<li>
<p><a>Set the configuration</a> specified by
<p>Let <var>connection</var> have a <dfn>[[Configuration]]</dfn>
Copy link
Contributor

@taylor-b taylor-b Jun 29, 2017

Choose a reason for hiding this comment

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

Needs to be [[\Configuration]] (with backslash), otherwise this gets interpreted as a bibliography link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@aboba aboba merged commit cbd1b11 into w3c:master Jul 6, 2017
@fippo fippo deleted the move-setconfiguration branch July 6, 2017 14:38
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.

editorial: move "set a configuration" to operation section
5 participants