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

Clear disabled signups message #374

Merged

Conversation

nghduc97
Copy link
Contributor

@nghduc97 nghduc97 commented Sep 6, 2024

The current behavior to disable only the button when DISABLE_SIGNUPS=true is really easy to misunderstood #52

Copy link
Collaborator

@MohamedBassem MohamedBassem left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the delay in review. The change itself looks ok to me, just requested a simpler way to achieve the same thing.

@@ -208,7 +207,6 @@ function SignUp() {
<ActionButton
type="submit"
loading={form.formState.isSubmitting}
disabled={clientConfig.auth.disableSignups}
>
Sign Up
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we just do the change in the text directly here?:

Suggested change
Sign Up
{clientConfig.auth.disableSignups ? "Signups are currently disabled" : "Sign Up"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hiding the whole signup form or tab would be much clearer, I don't think we need the form shown if it serve no purposes other than confusing users.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, this also confused me when i had that set for testing purposes. If signup is disabled, I don't expect to see a signup page

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, in that case I don't mind.

@MohamedBassem MohamedBassem merged commit 34fb5fb into hoarder-app:main Sep 15, 2024
3 checks passed
@MohamedBassem
Copy link
Collaborator

Thanks a lot @nghduc97 and welcome to the contributor lists!

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