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

UI for audio mute, video mute, camera switch, fullscreen and hangup #311

Merged
merged 29 commits into from
Jan 9, 2015

Conversation

samdutton
Copy link
Contributor

Uses inline SVGs to avoid slowing down page load. (These are well supported.)

Also added:
$() function for document.querySelector()
hide() and show() functions to add/remove the class name hidden
activate() and deactivate() to do the same for active

Getting this error with jstdPhantom Grunt task, not sure why:

Safari: Reset
Safari: Reset
...
Total 3 tests (Passed: 3; Fails: 0; Errors: 0) (4.00 ms)
Safari 534.34 Mac OS: Run 4 tests (Passed: 3; Fails: 0; Errors 1) (4.00 ms)
error loading file: /test/js/main.js:60: TypeError: 'null' is not an object (evaluating 'muteAudioSvg.onclick = toggleAudioMute')

>> Total Passed: 3, Fails: 0

PhantomJS threw an error:
Done, without errors.


</div>

<script type="text/javascript" src="/_ah/channel/jsapi"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

this should disappear now that we are using websockets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@juberti
Copy link
Contributor

juberti commented Dec 18, 2014

Looks solid, left a few comments. For future reference, please squash large commit sequences like this before sending a PR.

@jiayliu PTAL as well

[Complete video chat client (based on Google App Engine)](https://apprtc.appspot.com)
[AppRTC video chat client](https://apprtc.appspot.com)

[AppRTC parameters](https://googlechrome.github.io/webrtc/samples/web/content/apprtc/params.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably point to apprtc.appspot.com/params.html, to avoid confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@samdutton
Copy link
Contributor Author

New year's @juberti ping!

@@ -66,7 +66,9 @@ Patches and issues welcome!

### Video chat ###

[Complete video chat client (based on Google App Engine)](https://apprtc.appspot.com)
[AppRTC video chat client](https://apprtc.appspot.com)
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the "powered by Google App Engine" thing here - this is a continual source of confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in next commit.

@juberti
Copy link
Contributor

juberti commented Jan 9, 2015

At this point I just want to land this so we can avoid mergedowns against the other pending CLs. IOW I just want to check this in Friday; I will make any remaining fixes manually if needed.

@samdutton
Copy link
Contributor Author

@juberti ptal

@juberti
Copy link
Contributor

juberti commented Jan 9, 2015

lgtm (still curious about how you solved the flipped-buttons problem)

juberti added a commit that referenced this pull request Jan 9, 2015
UI for audio mute, video mute, camera switch, fullscreen and hangup
@juberti juberti merged commit f56e65a into webrtc:master Jan 9, 2015
@jiayliu
Copy link
Contributor

jiayliu commented Jan 12, 2015

One issue in main.js:

It calls isFullScreen, which is not defined anywhere.
https://github.com/GoogleChrome/webrtc/blob/master/samples/web/content/apprtc/js/main.js#L377

@samdutton
Copy link
Contributor Author

The shim code was meant to be moved to util.js, but somehow I missed that.

Fixed in commit in github.com//pull/358.

On Mon, Jan 12, 2015 at 11:30 PM, Jiayang Liu notifications@github.com
wrote:

One issue in main.js:

It calls isFullScreen, which is not defined anywhere.

https://github.com/GoogleChrome/webrtc/blob/master/samples/web/content/apprtc/js/main.js#L377


Reply to this email directly or view it on GitHub
#311 (comment).

This pull request was closed.
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