Support registration & login with phone number by dbkr · Pull Request #742 · matrix-org/matrix-react-sdk

dbkr · 2017-03-07T17:06:41Z

A lot of this is adding a CountryDropdown component to select the country for a phone number, and a Dropdown component on which its based. Note that some of the code comes from NetworkDropdown which has not yet been refactored to use Dropdown.

Adds phone number entry fields to login & registration screens
Adds a UI Auth entry component for m.login.msisdn auth
Changes login API to support phone numbers and use new typed-object style identifiers: https://docs.google.com/document/d/1-6ZSSW5YvCGhVFDyD2QExAUAdpCWjccvJT5xiyTTG2Y/edit#

Requires matrix-org/matrix-js-sdk#384
Requires matrix-org/synapse#1971
Requires: element-hq/element-web#3381

Stop the guest sync, otherwise it gets 401ed for using a guest access token for a non-guest, causing us to beliebe we've been logged out.

Now that Signup contains no code whatsoever related to signing up, rename it to Login. Get rid of the Signup class.

Rendering the whole lot when the component was rendered just makes the page load really slow, so just show 2 at a time and rely on type-to-search. Make type-to-search always display an exact iso2 match first

richvdh · 2017-03-08T14:00:52Z

    restProps.onClick = onClick;
-    restProps.onKeyDown = function(e) {
-        if (e.keyCode == 13 || e.keyCode == 32) return onClick();
+    restProps.onKeyUp = function(e) {


why is this changing, ooi?

Because I was listening for onKeyUp further up the tree and wondering why it wasn't being correctly captured by the AccessibleButton when the focus was there, and it was because it's on KeyDown, so since its generally more correct to use KeyUp than KeyDown, I changed this one. It also was not passing the event through to the onClick handler which was breaking things.

richvdh · 2017-03-08T14:13:19Z

+    componentWillReceiveProps(nextProps) {
+        this._reindexChildren(nextProps.children);
+        this.setState({
+            highlightedOption: Object.keys(this.childrenByKey)[0],


does this not choose a random child?

richvdh · 2017-03-08T14:18:08Z

+            </div>
+        }
+
+        const menuOptions = this._getMenuOptions();


Yes, yes it was

richvdh · 2017-03-08T14:19:00Z

+            <AccessibleButton className="mx_Dropdown_input" onClick={this._onInputClick}>
+                {currentValue}
+                <span className="mx_Dropdown_arrow"></span>
+                {menu}


should this be outside the AccessibleButton?

Have commented on why its done this way.

richvdh · 2017-03-08T14:20:56Z

+
+class MenuOption extends React.Component {
+    constructor(props) {
+        super(props)


richvdh · 2017-03-08T14:38:03Z

 }

+/**
+ * @param unicode {integer} One or more integers representing unicode characters


this needs much better documentation about what it does and when it is useful.

richvdh · 2017-03-08T14:40:43Z

+
+const MAX_DISPLAYED_ROWS = 2;
+
+export default class CountryDropdown extends React.Component {


haven't we decided to stick with old-style React.createClass classes for now, btw?

Yeah, seems so: this makes it consistent with Dropdown which was based on NetworkDropdown though.

richvdh · 2017-03-08T14:42:42Z

+
+    propTypes: {
+        submitAuthDict: React.PropTypes.func.isRequired,
+    },


this is a bit sparse.

richvdh · 2017-03-08T14:44:13Z

+        this.props.matrixClient.submitMsisdnToken(
+            this._sid, this.props.clientSecret, this.state.token
+        ).catch((e) => {
+            this.props.fail(e);


this will fall into the then block below with result === undefined, which will cause more exceptions?

ah yep. done.

richvdh · 2017-03-08T14:47:50Z

    },

+    onPhoneCountryChanged: function(country) {
+        this.setState({phoneCountry: country});


again, why are we tracking this (and phoneNumber) as a separate bit of state, rather than passing it in as a prop? it violates the keep-your-state-in-one-place principle of react.

fixed above

Not the array of object keys which has no particular order

dbkr · 2017-03-08T16:58:38Z

ptal

richvdh · 2017-03-08T17:50:41Z

-            selectedOption: this.props.initialSelectedOption || props.children[0].key,
            // The key of the highlighted option
            // (the option that would become selected if you pressed enter)
            highlightedOption: props.children[0].key,


this will blow up if children isn't an array, or is empty - eg on empty search results? we should probably do something better.

richvdh · 2017-03-08T17:50:42Z

        this._reindexChildren(nextProps.children);
        this.setState({
-            highlightedOption: Object.keys(this.childrenByKey)[0],
+            highlightedOption: nextProps.children[0].key,


richvdh · 2017-03-08T17:57:59Z

                    errorText: "Token incorrect",
                });
            }
+        }).catch((e) => {


this feels weird.

either move this.props.fail(e) to here, or replace catch(...).then(...).catch(...) with then(..., ...).

dbkr · 2017-03-08T18:52:35Z

ptal

Assuming [0] of an empty list == undefined doesn't work if you're then taking a property of it.

richvdh

lgtm

This reverts commit 0269562. This breaks against the current synapse release. We need to think more carefully about backwards compatibility.

Changes from #742

Split npm start into an init and watch script

dbkr added 24 commits February 2, 2017 10:47

WIP msisdn sign in

c1aea90

A mostly working country picker

69b2243

Merge remote-tracking branch 'origin/develop' into dbkr/msisdn_signin

85625f1

Fix bug where you'dbe logged out after registering

aa6871e

Stop the guest sync, otherwise it gets 401ed for using a guest access token for a non-guest, causing us to beliebe we've been logged out.

Use InteractiveAuth component for registration

4a0e17c

Fix tests

fce6842

Merge remote-tracking branch 'origin/develop' into dbkr/msisdn_signin

3f882d2

Remove old signup code

6c57c05

Signup -> Login

f8b910c

Now that Signup contains no code whatsoever related to signing up, rename it to Login. Get rid of the Signup class.

Merge branch 'dbkr/register_ui_auth' into dbkr/msisdn_signin

18b4eca

Stray newline

5b65f53

Fix more merge failing

173b578

Merge branch 'dbkr/register_ui_auth' into dbkr/msisdn_signin

3129101

Merge branch 'dbkr/register_ui_auth' into dbkr/msisdn_signin

7110eca

Get phone country & number to the right place

15b5cf5

Merge branch 'dbkr/register_ui_auth' into dbkr/msisdn_signin

c021032

Merge branch 'dbkr/register_ui_auth' into dbkr/msisdn_signin

e92b452

Merge remote-tracking branch 'origin/develop' into dbkr/msisdn_signin

127ace2

More-or-less working msisdn auth component

80f13e8

Send the bind_msisdn param on registration

404a660

Merge remote-tracking branch 'origin/develop' into dbkr/msisdn_signin

6ccd809

Refinements to country dropdown

24274c2

Rendering the whole lot when the component was rendered just makes the page load really slow, so just show 2 at a time and rely on type-to-search. Make type-to-search always display an exact iso2 match first

Propagate initial inputs to the phone input

6925a51

Support msisdn login

fdb8523

dbkr assigned richvdh Mar 7, 2017

richvdh suggested changes Mar 8, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 8, 2017

dbkr added 2 commits March 8, 2017 15:12

semicolon

234adbf

Fix PropTypes

510b9c5

dbkr added 8 commits March 8, 2017 15:17

Oops, use the 1qst element of the array

cb2fae6

Not the array of object keys which has no particular order

Make dropdown/countrydropdown controlled

fc2e414

Unused line

87c65fb

Add note on DOM layout

8e06836

onOptionChange is required

85bb935

More docs

807ddaa

Add missing propTypes

e0a3c7c

Don't resume promise on error

4481e8c

dbkr assigned richvdh and unassigned dbkr Mar 8, 2017

richvdh reviewed Mar 8, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 8, 2017

dbkr added 2 commits March 8, 2017 18:46

Use React.Children to manipulate children

83dd93f

Make catch less weird

740635d

dbkr assigned richvdh and unassigned dbkr Mar 8, 2017

Fix null dereference

2c4dfee

Assuming [0] of an empty list == undefined doesn't work if you're then taking a property of it.

richvdh approved these changes Mar 8, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 8, 2017

dbkr merged commit 0269562 into develop Mar 9, 2017

richvdh added a commit that referenced this pull request Mar 9, 2017

Revert "Support registration & login with phone number (#742)"

2786fb0

This reverts commit 0269562. This breaks against the current synapse release. We need to think more carefully about backwards compatibility.

dbkr added a commit that referenced this pull request Mar 14, 2017

Support msisdn signin

878413f

Changes from #742

This was referenced Mar 14, 2017

Login / registration with phone number, mark 2 #749

Closed

Login / registration with phone number, mark 2 #750

Merged

dbkr mentioned this pull request Jan 3, 2018

Dialog a11y #1652

Merged

martindale pushed a commit to FabricLabs/matrix-react-sdk that referenced this pull request Dec 8, 2018

Merge pull request matrix-org#742 from matrix-org/travis/build-process

3e32f47

Split npm start into an init and watch script


		const MAX_DISPLAYED_ROWS = 2;

		export default class CountryDropdown extends React.Component {

Uh oh!

Conversation

dbkr commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkr commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkr commented Mar 8, 2017

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dbkr commented Mar 7, 2017 •

edited

Loading