Add Electron Web Sample #7
Conversation
add Electron app
update readme
msach22
changed the title
add opentok client via npm
msach22
changed the title
update readme
Electron-Basic-Video-Chat/README.md
Outdated
|
|
||
| The Session object dispatches a `streamDestroyed` event when the stream is Destroyed. The application defines an event handler for this event: | ||
|
|
||
| session.on({ |
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/README.md
Outdated
| Once the Publisher object is initialized, we publish to the session using the `publish()` | ||
| method of the Session object: | ||
|
|
||
| session.publish(publisher, (error) => { |
There was a problem hiding this comment.
You should also escape this into a code block.
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/README.md
Outdated
|
|
||
| * When starting the application, you'll notice a silent error: | ||
| `ERROR:audio_send_stream.cc(93)] Failed to set up send codec state.` | ||
| This is a known [Electron issue](https://github.com/electron/electron/issues/8991). |
There was a problem hiding this comment.
Can you add next steps for the reader? If it's a known issue, what do they do next? e.g. wait for electron team to resolve, a workaround, restart?
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/app.js
Outdated
| })); | ||
|
|
||
| // Open the DevTools. | ||
| // mainWindow.webContents.openDevTools() |
There was a problem hiding this comment.
Remove this if you aren't using it as part of the sample.
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/app.js
Outdated
| // Dereference the window object, usually you would store windows | ||
| // in an array if your app supports multi windows, this is the time | ||
| // when you should delete the corresponding element. | ||
| mainWindow = null |
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/openTok.js
Outdated
| session.on({ | ||
| streamCreated: (event) => { | ||
| session.subscribe(event.stream, 'subscriber', (error) => { | ||
| if (error) console.log(`There was an issue subscribing to the stream ${event}`); |
There was a problem hiding this comment.
if brace style should be consistent - if you do one-liners, use one-liners always when possible. If you don't, always use brackets. There's some file-wide variation that can be confusing for someone who is unfamiliar with the language.
IMHO I would recommend brackets always - the default ESLint style can be found here: https://eslint.org/docs/rules/brace-style#options
e.g.
if (cond) {
action();
}
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/openTok.js
Outdated
| }); | ||
| }, | ||
| streamDestroyed: (event) => { | ||
| console.log(`Stream ${event.stream.name} ended ${event.reason}`); |
There was a problem hiding this comment.
I think some better sentence structure could go a long way for the console logs when you're interpolating event params in this sample repo. Otherwise the intent of the injection is not clear.
e.g. Stream with name: ${name} ended because of reason: ${reason}
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/openTok.js
Outdated
| session.connect(token, (error) => { | ||
| // If the connection is successful, initialize a publisher and publish to the session | ||
| if (error) { | ||
| return console.log('error connecting to session', error); |
There was a problem hiding this comment.
Can you also add spacing like 'Error connecting to session: ' + error.reason?
Otherwise you'd get "error connection to sessionError: eventReasonHere" which is less readable.
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/openTok.js
Outdated
| //Initialize Session | ||
| const session = OT.initSession(apiKey, sessionId); | ||
|
|
||
| //Set session event listeners |
There was a problem hiding this comment.
fixed in the latest commit
Electron-Basic-Video-Chat/openTok.js
Outdated
| const sessionId = ""; | ||
| const token = ""; | ||
|
|
||
| //Initialize Session |
There was a problem hiding this comment.
fixed in the latest commit
getting ready for public
Electron-Basic-Video-Chat/openTok.js
Outdated
| session.on({ | ||
| streamCreated: (event) => { | ||
| session.subscribe(event.stream, 'subscriber', (error) => { | ||
| if (error) console.log(`There was an issue subscribing to the stream ${event}`); | ||
| if (error) { | ||
| console.log(`There was an issue subscribing to the stream ${event}`); |
There was a problem hiding this comment.
This seems wrong: why are we logging the event instead of the error?
Can you also fix the aforementioned stream with error: ${error} concat?
Electron-Basic-Video-Chat/README.md
Outdated
| This is a known [Electron issue](https://github.com/electron/electron/issues/8991). | ||
| `ERROR:audio_send_stream.cc(93)] Failed to set up send codec state.` | ||
| This is a known [Electron issue](https://github.com/electron/electron/issues/8991). | ||
| We recommend staying up to date with the issue to see if the Electron team or other contributors have a solution |
Electron-Basic-Video-Chat/README.md
Outdated
| return console.log('There was an error publishing', error); | ||
| } | ||
| }); | ||
| session.publish(publisher, (error) => { |
There was a problem hiding this comment.
This is still not in a code block.
Open the file in the Github UI to see the rendered markdown view.
There was a problem hiding this comment.
i read your comment incorrectly my b, fixing in next commit
No description provided.