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

ui: control signup form visibility with configs #169

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

audrium
Copy link
Member

@audrium audrium commented Dec 17, 2020

addresses reanahub/reana#469

@audrium audrium requested a review from mvidalgarcia December 17, 2020 11:47
@audrium audrium self-assigned this Dec 17, 2020
if (config.localUsers && signup) {
return (
<p>
Already signed up? Go to <Link to="/signin">Sign In</Link>
Copy link
Member

Choose a reason for hiding this comment

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

Minor: In order to keep consistency with "Sign up".

Suggested change
Already signed up? Go to <Link to="/signin">Sign In</Link>
Already signed up? Go to <Link to="/signin">Sign in</Link>

)}
<SignFormInfo config={config} signup />
Copy link
Member

Choose a reason for hiding this comment

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

I agree that SignFormInfo component looks a bit convoluted and prints completely different things. What if we keep Signup component as it was and disable the /singup route if hideSignup is true?
Then we can have the logic of SingFormInfo inside Signin component.

Copy link
Member Author

Choose a reason for hiding this comment

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

This approach looks cleaner, but there is one downside. Currently in the App.js we are waiting for the /api/you response before rendering any pages. If we want to disable the route in this place we will have to wait for /api/config to arrive, since hideSignup is stored there. So +1 request to wait for the user before seeing some content. Not sure if this is a big problem, probably noticeable only on a very slow connection, but still something to think about. Because of this reason I took another approach which is not very clean.. Could be that sooner or later we will still have to depend on the configs to render or not some component or page.

Copy link
Member Author

Choose a reason for hiding this comment

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

After playing around a bit, I think it's still better to disable the route, so I changed the code according to your suggestions

@diegodelemos diegodelemos merged commit 451c817 into reanahub:maint-0.7 Jan 6, 2021
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.

3 participants