Split out InterActiveAuthDialog #691
Conversation
Into a component that does Interactive Auth and a dialog that wraps it, so we can do interactive auth not necessarily in a dialog. As a side effect: * Put the buttons for each auth stage in the stage itself. Some stages don't have submit buttons (and it's very possible other stages may have other buttons entirely, like 'resend') so it makes more sense for the buttons to live in the stage components themselves. Plus it saves the slightly evil calling-functions-on-react-children thing we were doing (and indeed extending that to show the submit button at all). * Give all BaseDialogs a cross in the top right to cancel. They were all dismissable by clicking outside or pressing esc, so this adds a more visually obvious way of dismissing them. Plus, it means our InteractiveAuthDialog can have a way of canceling the whole operation separate from buttons for the individual stages.
| /> | ||
| <div className="mx_button_row"> | ||
| <input type="submit" | ||
| className="mx_Dialog_primary" |
There was a problem hiding this comment.
Using the mx_Dialog class here: the button classes need refactoring into a single class, so rather than c+p the CSS yet again, I'm reusing this class.
| onChange={this._onPasswordFieldChange} | ||
| type="password" | ||
| /> | ||
| <form onSubmit={this._onSubmit}> |
There was a problem hiding this comment.
I've put this in a form which makes the behaviour more semantic and accessible, (we then get submit-on-enter for free, etc)
| }, | ||
| | ||
| render: function() { | ||
| const Loader = sdk.getComponent("elements.Spinner"); |
There was a problem hiding this comment.
... except I can't see any feedback to the user that a request is in-flight. We used to show a spinner in place of the submit button when state.busy was true (which was ugly as hell imho, but still). We ought to reinstate this somehow.
| }, | ||
| | ||
| _onCancelClick: function(e) { | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
think e.stopPropagation(), e.preventDefault() are pretty unnecessary here
| } | ||
| }, | ||
| | ||
| _onCancel: function() { |
There was a problem hiding this comment.
is this ever used? it looks superflous
| // callback | ||
| makeRequest: React.PropTypes.func.isRequired, | ||
| | ||
| onFinished: React.PropTypes.func.isRequired, |
There was a problem hiding this comment.
could you document how this is used, given it is no longer necessarily part of the Modal stuff with its implied convention?
| }, | ||
| | ||
| _onCancel: function() { | ||
| _onCancelClick: function() { |
| * stageParams: params from the server for the stage being attempted | ||
| * errorText: error message from a previous attempt to authenticate | ||
| * submitAuthDict: a function which will be called with the new auth dict | ||
| * setSubmitButtonEnabled: a function which will enable/disable the 'submit' button |
There was a problem hiding this comment.
github won't let me comment on it, but could you update the comment on line 24 to refer to InteractiveAuth rather than InteractiveAuthDialog?
| * setSubmitButtonEnabled: a function which will enable/disable the 'submit' button | ||
| * | ||
| * Each component may also provide the following functions (beyond the standard React ones): | ||
| * onSubmitClick: handle a 'submit' button click |
There was a problem hiding this comment.
Yeah, I moved the submit button into the stage components, so no need for them to be passed a function to disable one somewhere else.
There was a problem hiding this comment.
Oh, I see what you mean, yes. (Still find myself thinking the line you've commented on is the line I changed rather than the one at the bottom of the diff).
| // we have to make the user click a button, as browsers will block | ||
| // the popup if we open it immediately. | ||
| this._popupWindow = null; | ||
| this.props.setSubmitButtonEnabled(true); |
| }, | ||
| | ||
| render: function() { | ||
| const Loader = sdk.getComponent("elements.Spinner"); |
There was a problem hiding this comment.
... except I can't see any feedback to the user that a request is in-flight. We used to show a spinner in place of the submit button when state.busy was true (which was ugly as hell imho, but still). We ought to reinstate this somehow.
and update test accordingly
| Have also added a spinner |
| errorText: React.PropTypes.string, | ||
| // is the auth logic currently waiting for something to | ||
| // happen? | ||
| busy: React.PropTypes.bool, |
There was a problem hiding this comment.
Add this to the list of properties at the top of the file?
| looks good apart from a couple of nits |
Into a component that does Interactive Auth and a dialog that
wraps it, so we can do interactive auth not necessarily in a
dialog.
As a side effect:
Some stages don't have submit buttons (and it's very possible
other stages may have other buttons entirely, like 'resend')
so it makes more sense for the buttons to live in the stage
components themselves. Plus it saves the slightly evil
calling-functions-on-react-children thing we were doing (and
indeed extending that to show the submit button at all).
were all dismissable by clicking outside or pressing esc, so
this adds a more visually obvious way of dismissing them. Plus,
it means our InteractiveAuthDialog can have a way of canceling
the whole operation separate from buttons for the individual
stages.
Requires element-hq/element-web#3217