-
-
Notifications
You must be signed in to change notification settings - Fork 832
Show UDDialog on UDE during VoIP calls #721
Conversation
Requires matrix-org/matrix-js-sdk#378 Also, refactored UDDialog creation into its own dispatch event, because there will be other parts of the code that will want to spawn one.
@@ -518,6 +519,32 @@ module.exports = React.createClass({ | |||
case 'set_theme': | |||
this._onSetTheme(payload.value); | |||
break; | |||
case 'unknown_device_error': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for making these dispatched actions rather than just utility functions or something? Dispatching actions is normally to make MatrixClient or some higher level app component do something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could easily be a utility. It would be nicer to have a DialogSpawner
component or something, but this wouldn't be visible. I'm slightly concerned about dumping this in src
. I wished files were grouped by function (like the autocomplete
and login
directories (sadly there are two login
folders)) and not by category like elements
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the dispatch seems a bit redundant - maybe better to just have a function in Resend.js. Also, should probably s/var/const/ on code when we move it.
This makes sense. What about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than that, OK - lgtm
src/UnknownDeviceErrorHandler.js
Outdated
|
||
let ref = null; | ||
|
||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not mix ES6 import with commonjs export syntax: export function startListening() {
would be better (which probably means you'll need to change the places its required to be import { startListening, stopListening } from '../../UnknownDeviceErrorHandler'
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, sgtm
Requires matrix-org/matrix-js-sdk#378 ✅
Also, refactored UDDialog creation into its own dispatch event, because there will be other parts of the code that will want to spawn one.
Fixes element-hq/element-web#3285