From 438558da55fdb8ba67e55a8a3cf65a8d33caecb4 Mon Sep 17 00:00:00 2001
From: Matthew Hodgson <matthew@matrix.org>
Date: Tue, 15 Mar 2016 13:48:46 +0000
Subject: [PATCH] PR feedback

---
 src/components/structures/MatrixChat.js       | 12 ++---
 .../structures/login/ForgotPassword.js        |  8 ++--
 src/components/structures/login/Login.js      | 12 ++---
 .../structures/login/Registration.js          | 10 ++---
 src/components/views/login/ServerConfig.js    | 44 +++++++++++++------
 5 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index f4337e15fd..6d74b4ab12 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1003,8 +1003,8 @@ module.exports = React.createClass({
                     guestAccessToken={this.state.guestAccessToken}
                     defaultHsUrl={this.props.config.default_hs_url}
                     defaultIsUrl={this.props.config.default_is_url}
-                    initialHsUrl={this.getCurrentHsUrl()}
-                    initialIsUrl={this.getCurrentIsUrl()}
+                    customHsUrl={this.getCurrentHsUrl()}
+                    customIsUrl={this.getCurrentIsUrl()}
                     registrationUrl={this.props.registrationUrl}
                     onLoggedIn={this.onRegistered}
                     onLoginClick={this.onLoginClick}
@@ -1015,8 +1015,8 @@ module.exports = React.createClass({
                 <ForgotPassword
                     defaultHsUrl={this.props.config.default_hs_url}
                     defaultIsUrl={this.props.config.default_is_url}
-                    initialHsUrl={this.getCurrentHsUrl()}
-                    initialIsUrl={this.getCurrentIsUrl()}
+                    customHsUrl={this.getCurrentHsUrl()}
+                    customIsUrl={this.getCurrentIsUrl()}
                     onComplete={this.onLoginClick}
                     onLoginClick={this.onLoginClick} />
             );
@@ -1027,8 +1027,8 @@ module.exports = React.createClass({
                     onRegisterClick={this.onRegisterClick}
                     defaultHsUrl={this.props.config.default_hs_url}
                     defaultIsUrl={this.props.config.default_is_url}
-                    initialHsUrl={this.getCurrentHsUrl()}
-                    initialIsUrl={this.getCurrentIsUrl()}
+                    customHsUrl={this.getCurrentHsUrl()}
+                    customIsUrl={this.getCurrentIsUrl()}
                     onForgotPasswordClick={this.onForgotPasswordClick} 
                     onLoginAsGuestClick={this.props.enableGuest && this.props.config && this.props.config.default_hs_url ? this._registerAsGuest: undefined}
                     />
diff --git a/src/components/structures/login/ForgotPassword.js b/src/components/structures/login/ForgotPassword.js
index 7f22ee41f1..c457ce1ead 100644
--- a/src/components/structures/login/ForgotPassword.js
+++ b/src/components/structures/login/ForgotPassword.js
@@ -29,8 +29,8 @@ module.exports = React.createClass({
     propTypes: {
         defaultHsUrl: React.PropTypes.string,
         defaultIsUrl: React.PropTypes.string,
-        initialHsUrl: React.PropTypes.string,
-        initialIsUrl: React.PropTypes.string,
+        customHsUrl: React.PropTypes.string,
+        customIsUrl: React.PropTypes.string,
         onLoginClick: React.PropTypes.func,
         onRegisterClick: React.PropTypes.func,
         onComplete: React.PropTypes.func.isRequired
@@ -182,8 +182,8 @@ module.exports = React.createClass({
                         withToggleButton={true}
                         defaultHsUrl={this.props.defaultHsUrl}
                         defaultIsUrl={this.props.defaultIsUrl}
-                        initialHsUrl={this.props.initialHsUrl}
-                        initialIsUrl={this.props.initialIsUrl}
+                        customHsUrl={this.props.customHsUrl}
+                        customIsUrl={this.props.customIsUrl}
                         onHsUrlChanged={this.onHsUrlChanged}
                         onIsUrlChanged={this.onIsUrlChanged}
                         delayTimeMs={0}/>
diff --git a/src/components/structures/login/Login.js b/src/components/structures/login/Login.js
index 9ab943bb68..1b881187f8 100644
--- a/src/components/structures/login/Login.js
+++ b/src/components/structures/login/Login.js
@@ -31,8 +31,8 @@ module.exports = React.createClass({displayName: 'Login',
     propTypes: {
         onLoggedIn: React.PropTypes.func.isRequired,
 
-        initialHsUrl: React.PropTypes.string,
-        initialIsUrl: React.PropTypes.string,
+        customHsUrl: React.PropTypes.string,
+        customIsUrl: React.PropTypes.string,
         defaultHsUrl: React.PropTypes.string,
         defaultIsUrl: React.PropTypes.string,
 
@@ -48,8 +48,8 @@ module.exports = React.createClass({displayName: 'Login',
         return {
             busy: false,
             errorText: null,
-            enteredHomeserverUrl: this.props.initialHsUrl || this.props.defaultHsUrl,
-            enteredIdentityServerUrl: this.props.initialIsUrl || this.props.defaultIsUrl,
+            enteredHomeserverUrl: this.props.customHsUrl || this.props.defaultHsUrl,
+            enteredIdentityServerUrl: this.props.customIsUrl || this.props.defaultIsUrl,
 
             // used for preserving username when changing homeserver
             username: "",
@@ -220,8 +220,8 @@ module.exports = React.createClass({displayName: 'Login',
                         { this.componentForStep(this._getCurrentFlowStep()) }
                         <ServerConfig ref="serverConfig"
                             withToggleButton={true}
-                            initialHsUrl={this.props.initialHsUrl}
-                            initialIsUrl={this.props.initialIsUrl}
+                            customHsUrl={this.props.customHsUrl}
+                            customIsUrl={this.props.customIsUrl}
                             defaultHsUrl={this.props.defaultHsUrl}
                             defaultIsUrl={this.props.defaultIsUrl}
                             onHsUrlChanged={this.onHsUrlChanged}
diff --git a/src/components/structures/login/Registration.js b/src/components/structures/login/Registration.js
index 955f90774b..cf9adee1bb 100644
--- a/src/components/structures/login/Registration.js
+++ b/src/components/structures/login/Registration.js
@@ -36,8 +36,8 @@ module.exports = React.createClass({
         sessionId: React.PropTypes.string,
         registrationUrl: React.PropTypes.string,
         idSid: React.PropTypes.string,
-        initialHsUrl: React.PropTypes.string,
-        initialIsUrl: React.PropTypes.string,
+        customHsUrl: React.PropTypes.string,
+        customIsUrl: React.PropTypes.string,
         defaultHsUrl: React.PropTypes.string,
         defaultIsUrl: React.PropTypes.string,
         email: React.PropTypes.string,
@@ -59,7 +59,7 @@ module.exports = React.createClass({
         this.dispatcherRef = dis.register(this.onAction);
         // attach this to the instance rather than this.state since it isn't UI
         this.registerLogic = new Signup.Register(
-            this.props.initialHsUrl, this.props.initialIsUrl
+            this.props.customHsUrl, this.props.customIsUrl
         );
         this.registerLogic.setClientSecret(this.props.clientSecret);
         this.registerLogic.setSessionId(this.props.sessionId);
@@ -242,8 +242,8 @@ module.exports = React.createClass({
                 {busySpinner}
                 <ServerConfig ref="serverConfig"
                     withToggleButton={true}
-                    initialHsUrl={this.props.initialHsUrl}
-                    initialIsUrl={this.props.initialIsUrl}
+                    customHsUrl={this.props.customHsUrl}
+                    customIsUrl={this.props.customIsUrl}
                     defaultHsUrl={this.props.defaultHsUrl}
                     defaultIsUrl={this.props.defaultIsUrl}
                     onHsUrlChanged={this.onHsUrlChanged}
diff --git a/src/components/views/login/ServerConfig.js b/src/components/views/login/ServerConfig.js
index 94b9d73f2f..fd487b25ef 100644
--- a/src/components/views/login/ServerConfig.js
+++ b/src/components/views/login/ServerConfig.js
@@ -29,10 +29,21 @@ module.exports = React.createClass({
     propTypes: {
         onHsUrlChanged: React.PropTypes.func,
         onIsUrlChanged: React.PropTypes.func,
-        initialHsUrl: React.PropTypes.string, // whatever the current value is when we create the component
-        initialIsUrl: React.PropTypes.string, // whatever the current value is when we create the component
+
+        // default URLs are defined in config.json (or the hardcoded defaults)
+        // they are used if the user has not overridden them with a custom URL.
+        // In other words, if the custom URL is blank, the default is used.
         defaultHsUrl: React.PropTypes.string, // e.g. https://matrix.org
         defaultIsUrl: React.PropTypes.string, // e.g. https://vector.im
+
+        // custom URLs are explicitly provided by the user and override the
+        // default URLs.  The user enters them via the component's input fields,
+        // which is reflected on these properties whenever on..UrlChanged fires.
+        // They are persisted in localStorage by MatrixClientPeg, and so can
+        // override the default URLs when the component initially loads.
+        customHsUrl: React.PropTypes.string,
+        customIsUrl: React.PropTypes.string,
+
         withToggleButton: React.PropTypes.bool,
         delayTimeMs: React.PropTypes.number // time to wait before invoking onChanged
     },
@@ -48,12 +59,12 @@ module.exports = React.createClass({
 
     getInitialState: function() {
         return {
-            hs_url: this.props.initialHsUrl,
-            is_url: this.props.initialIsUrl,
+            hs_url: this.props.customHsUrl,
+            is_url: this.props.customIsUrl,
             // if withToggleButton is false, then show the config all the time given we have no way otherwise of making it visible
             configVisible: !this.props.withToggleButton || 
-                           (this.props.initialHsUrl !== this.props.defaultHsUrl) ||
-                           (this.props.initialIsUrl !== this.props.defaultIsUrl)
+                           (this.props.customHsUrl !== this.props.defaultHsUrl) ||
+                           (this.props.customIsUrl !== this.props.defaultIsUrl)
         }
     },
 
@@ -84,11 +95,11 @@ module.exports = React.createClass({
         return setTimeout(fn.bind(this), this.props.delayTimeMs);
     },
 
-    onServerConfigVisibleChange: function(ev) {
+    onServerConfigVisibleChange: function(visible, ev) {
         this.setState({
-            configVisible: ev.target.checked
+            configVisible: visible
         });
-        if (!ev.target.checked) {
+        if (!visible) {
             this.props.onHsUrlChanged(this.props.defaultHsUrl);
             this.props.onIsUrlChanged(this.props.defaultIsUrl);
         }
@@ -110,12 +121,19 @@ module.exports = React.createClass({
         var toggleButton;
         if (this.props.withToggleButton) {
             toggleButton = (
-                <div>
-                    <input className="mx_Login_checkbox" id="advanced" type="checkbox"
+                <div style={{ textAlign: 'center' }}>
+                    <input className="mx_Login_radio" id="basic" name="configVisible" type="radio"
+                        checked={!this.state.configVisible}
+                        onChange={this.onServerConfigVisibleChange.bind(this, false)} />
+                    <label className="mx_Login_label" htmlFor="basic">
+                        Default server
+                    </label>
+                    &nbsp;&nbsp;
+                    <input className="mx_Login_radio" id="advanced" name="configVisible" type="radio"
                         checked={this.state.configVisible}
-                        onChange={this.onServerConfigVisibleChange} />
+                        onChange={this.onServerConfigVisibleChange.bind(this, true)} />
                     <label className="mx_Login_label" htmlFor="advanced">
-                        Use custom server options (advanced)
+                        Custom server
                     </label>
                 </div>
             );