Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Show UDDialog on UDE during VoIP calls #721

Merged
merged 8 commits into from
Mar 2, 2017

Conversation

lukebarnard1
Copy link
Contributor

@lukebarnard1 lukebarnard1 commented Feb 21, 2017

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

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.
@lukebarnard1 lukebarnard1 changed the title Show UDDialog on m.call.invite failure Show UDDialog during VoIP calls Feb 22, 2017
@lukebarnard1 lukebarnard1 changed the title Show UDDialog during VoIP calls Show UDDialog on UDE during VoIP calls Feb 22, 2017
@@ -518,6 +519,32 @@ module.exports = React.createClass({
case 'set_theme':
this._onSetTheme(payload.value);
break;
case 'unknown_device_error':
Copy link
Member

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.

Copy link
Contributor Author

@lukebarnard1 lukebarnard1 Feb 22, 2017

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.

Copy link
Member

@dbkr dbkr left a 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.

@lukebarnard1
Copy link
Contributor Author

maybe better to just have a function in Resend.js

This makes sense. What about unknown_device_error dispatch? I guess UnknownDeviceErrorHandler could be replaced with UnknownDeviceDialogSpwaner ?

Copy link
Member

@dbkr dbkr left a 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


let ref = null;

module.exports = {
Copy link
Member

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').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, sgtm

@dbkr dbkr merged commit 95cff17 into develop Mar 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants