Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor registration #399

Merged
merged 14 commits into from
Nov 20, 2015
Merged

Refactor registration #399

merged 14 commits into from
Nov 20, 2015

Conversation

kegsay
Copy link
Contributor

@kegsay kegsay commented Nov 19, 2015

This is the sister PR to matrix-org/matrix-react-sdk#34

This PR factors out a lot of UI components from the Register component. The new components are:

  • CaptchaForm - Renders the div which Recaptcha needs. Does the componentDidUpdate script voodoo to make it actually work.
  • RegistrationForm - Contains the elements for the email/user/password. Does basic validation like checking passwords match/exist/length and calls the onError prop accordingly.

It adds a new wire component Registration which glues everything together. Specifically it:

  • Is responsible for configuring the logic class Signup.Register.
  • Is responsible for rendering the right login type (by delegating to those UI components like CaptchaForm).

There's a few things I'd like to do to improve this further, but I'm conscious of keeping PRs manageable so have littered a few TODOs.

Manually tested:

  • Password only registration (locally)
  • Password + Captcha registration (on matrix.org)
  • Password + Email + Captcha registration (on matrix.org)

Hook it up to MatrixChat instead of the existing logic (this breaks reg). WIP.
This is complex enough that the Registration component shouldn't have to
care about it, so it should probably be split into a pure UI component.
This has to be done rather than in MatrixChat because the render() calls
will create new instances otherwise. Pass in all the strings the logic class
requires to the Registration wire component. This isn't the "best" solution
because unloading/reloading the Registration component will lose registration
state which should be persisted. Ideally we'd DI from the top to ensure this
can't happen (as opposed to relying on module globals...)
@kegsay kegsay changed the title Kegan/reg refactor Refactor registration Nov 19, 2015
</div>
);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

This component look simple / generic enough it could potentially go into react-sdk in the long term?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, I should've seen that. Will re-jig now else I'll never get around to it.

@dbkr
Copy link
Member

dbkr commented Nov 19, 2015

lgtm

@dbkr dbkr assigned kegsay and unassigned dbkr Nov 19, 2015
kegsay added a commit that referenced this pull request Nov 20, 2015
@kegsay kegsay merged commit 99ccff0 into develop Nov 20, 2015
@t3chguy t3chguy deleted the kegan/reg-refactor branch May 12, 2022 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants