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

Allow Modal to be used with async-loaded components #618

Merged
merged 1 commit into from
Jan 19, 2017
Merged

Allow Modal to be used with async-loaded components #618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 71 additions & 2 deletions src/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,53 @@ limitations under the License.

var React = require('react');
var ReactDOM = require('react-dom');
import sdk from './index';

/**
* Wrap an asynchronous loader function with a react component which shows a
* spinner until the real component loads.
*/
const AsyncWrapper = React.createClass({
propTypes: {
/** A function which takes a 'callback' argument which it will call
* with the real component once it loads.
*/
loader: React.PropTypes.func.isRequired,
},

getInitialState: function() {
return {
component: null,
}
},

componentWillMount: function() {
this._unmounted = false;
this.props.loader((e) => {
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 callbacks rather than promises here, ooi?

Copy link
Member Author

Choose a reason for hiding this comment

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

just that require (or require.ensure, for that matter) takes a callback, rather than returning a promise. We can't easily wrap require with something that returns a promise, because that breaks webpack's static analysis. So if loader was expected to be a Promise, we would have to add some nasty boilerplate everywhere we loaded a component asynchronously.

IOW: currently we can do:

createDialogAsync((cb) => {require(['foo'], cb)});

If instead we had to pass a promise, this would become:

d = q.defer(); 
require(['foo'], (m) => {d.resolve(m)});
createDialogAsync(d.promise);

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, if it makes it simpler then callbacks ftw.

if (this._unmounted) {
return;
}
this.setState({component: e});
});
},

componentWillUnmount: function() {
this._unmounted = true;
},

render: function() {
const {loader, ...otherProps} = this.props;

if (this.state.component) {
const Component = this.state.component;
return <Component {...otherProps} />;
} else {
// show a spinner until the component is loaded.
const Spinner = sdk.getComponent("elements.Spinner");
return <Spinner />;
}
},
});

module.exports = {
DialogContainerId: "mx_Dialog_Container",
Expand All @@ -36,8 +83,30 @@ module.exports = {
},

createDialog: function (Element, props, className) {
var self = this;
return this.createDialogAsync((cb) => {cb(Element)}, props, className);
},

/**
* Open a modal view.
*
* This can be used to display a react component which is loaded as an asynchronous
* webpack component. To do this, set 'loader' as:
*
* (cb) => {
* require(['<module>'], cb);
* }
*
* @param {Function} loader a function which takes a 'callback' argument,
* which it should call with a React component which will be displayed as
* the modal view.
*
* @param {Object} props properties to pass to the displayed
* component. (We will also pass an 'onFinished' property.)
*
* @param {String} className CSS class to apply to the modal wrapper
*/
createDialogAsync: function (loader, props, className) {
var self = this;
// never call this via modal.close() from onFinished() otherwise it will loop
var closeDialog = function() {
if (props && props.onFinished) props.onFinished.apply(null, arguments);
Expand All @@ -49,7 +118,7 @@ module.exports = {
var dialog = (
<div className={"mx_Dialog_wrapper " + className}>
<div className="mx_Dialog">
<Element {...props} onFinished={closeDialog}/>
<AsyncWrapper loader={loader} {...props} onFinished={closeDialog}/>
</div>
<div className="mx_Dialog_background" onClick={ closeDialog.bind(this, false) }></div>
</div>
Expand Down
0 ...nts/views/dialogs/EncryptedEventDialog.js → ...nts/views/dialogs/EncryptedEventDialog.js
View file
Open in desktop
File renamed without changes.
2 changes: 0 additions & 2 deletions src/component-index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,6 @@ import views$dialogs$ChatInviteDialog from './components/views/dialogs/ChatInvit
views$dialogs$ChatInviteDialog && (module.exports.components['views.dialogs.ChatInviteDialog'] = views$dialogs$ChatInviteDialog);
import views$dialogs$DeactivateAccountDialog from './components/views/dialogs/DeactivateAccountDialog';
views$dialogs$DeactivateAccountDialog && (module.exports.components['views.dialogs.DeactivateAccountDialog'] = views$dialogs$DeactivateAccountDialog);
import views$dialogs$EncryptedEventDialog from './components/views/dialogs/EncryptedEventDialog';
views$dialogs$EncryptedEventDialog && (module.exports.components['views.dialogs.EncryptedEventDialog'] = views$dialogs$EncryptedEventDialog);
import views$dialogs$ErrorDialog from './components/views/dialogs/ErrorDialog';
views$dialogs$ErrorDialog && (module.exports.components['views.dialogs.ErrorDialog'] = views$dialogs$ErrorDialog);
import views$dialogs$InteractiveAuthDialog from './components/views/dialogs/InteractiveAuthDialog';
Expand Down
5 changes: 3 additions & 2 deletions src/components/views/rooms/EventTile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,10 +366,11 @@ module.exports = WithMatrixClient(React.createClass({
},

onCryptoClicked: function(e) {
var EncryptedEventDialog = sdk.getComponent("dialogs.EncryptedEventDialog");
var event = this.props.mxEvent;

Modal.createDialog(EncryptedEventDialog, {
Modal.createDialogAsync((cb) => {
require(['../../../async-components/views/dialogs/EncryptedEventDialog'], cb)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether it would be better to use the CommonJS require.ensure here, as we've used CommonJS elsewhere, rather than introducing AMD syntax (https://webpack.github.io/docs/code-splitting.html) (also require.ensure seems to me a little more obvious what it might be doing, rather than calling require with an array and a callback versus a string).

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, though where have we used commonjs elsewhere? (And it's the callback that makes the difference between the two types of require, rather than the arrayness, but yes)

will make it so

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, except that conflicts with my previous point: using the CommonJS syntax would be considerably more verbose, as there is no longer a simple way to pass the component into createDialogAsync.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we use straight 'require' which is CommonJS, although of course now we're moving to ES6 import syntax, but webpack doesn't support this for async loading. And yeah, it does seem to make it quite a chunk more verbose.

}, {
event: event,
});
},
Expand Down