-
Notifications
You must be signed in to change notification settings - Fork 24
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
Allow hotlink to organization-specific sign-up form #2898
Conversation
…of displayname for url; integrate invite-popover in user list view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice and clean :) I like your solution with the popover for the registration link 👍
onRegistered={() => { | ||
Toast.success(messages["auth.account_created"]); | ||
this.props.history.push("/auth/login"); | ||
}} | ||
onOrganizationNameNotFound={() => { | ||
Toast.error(messages["auth.invalid_organization_name"]); | ||
this.props.history.push("/auth/register"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding this error can only be triggered if the user already is on the register page. So I'd say there's no need to push this to the history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure I get what you mean. If this error happens, the user is on /auth/register?organizationName=someInvalidOrgName
. Then, he should be redirected to /auth/register
, so that the dropdown appears again. Or do you mean that something like history.replace
should be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you're right. I didn't get that this line was being used to "get rid" of the organizationName
url parameter :)
.substring(1) | ||
.split("&") | ||
.reduce((result, value): void => { | ||
.reduce((result: UrlParamsType, value: string): UrlParamsType => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
<Route | ||
path="/auth/register" | ||
render={({ location }: ContextRouter) => { | ||
const params = Utils.getUrlParamsObjectFromString(location.search); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you split up this method in the utils module, but then you're calling it with location.search
anyways? Couldn't you use the original method then? Maybe I'm missing something here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
location
is here passed from the router (see line 351). window.location
might have worked here, too, but to be precise, I used the value passed by react-router :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, now it makes sense :)
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
Screenshots
Updated migration guide if applicable