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

Feature/password reset #29

Open
wants to merge 4 commits into
base: release
Choose a base branch
from
Open

Feature/password reset #29

wants to merge 4 commits into from

Conversation

jtm13
Copy link
Contributor

@jtm13 jtm13 commented Jan 13, 2023

Handled the firebase error on issue #27

@jtm13 jtm13 changed the base branch from master to release January 13, 2023 20:43
@jtm13 jtm13 requested a review from genki-aik as a code owner January 13, 2023 20:43
@genki-aik
Copy link
Contributor

genki-aik commented Jan 13, 2023

React hook form actually does not re-render the page every input so I don't think the error message renders for me. I will take a look and make it re-render every time.

@genki-aik
Copy link
Contributor

It might be better to use FirebaseErrors instead of matching string errors, I will update you on that as well

@jtm13
Copy link
Contributor Author

jtm13 commented Jan 14, 2023

React hook form actually does not re-render the page every input so I don't think the error message renders for me. I will take a look and make it re-render every time.

If it does not work on your end, then I can find another way to make it work.

@jtm13
Copy link
Contributor Author

jtm13 commented Jan 14, 2023

It might be better to use FirebaseErrors instead of matching string errors, I will update you on that as well

I did think about doing that, but I choose the dirtier option since, in Typescript, you can only specify errors as any or unknown. Also, in JavaScript (and Typescript), there is only one catch as opposed to Java, where you can catch specific errors without much boilerplate. Since, when looking at the docs, there is no section in the method detail sendResetPasswordEmail where it discusses any errors it can throw, I assume it might also throw other general errors as well. In consideration of all of these, I decided to just make the code work for all types of errors. I can change it to only do FirebaseErrors though.

@jtm13
Copy link
Contributor Author

jtm13 commented Jan 14, 2023

It might be better to use FirebaseErrors instead of matching string errors, I will update you on that as well

I did think about doing that, but I choose the dirtier option since, in Typescript, you can only specify errors as any or unknown. Also, in JavaScript (and Typescript), there is only one catch as opposed to Java, where you can catch specific errors without much boilerplate. Since, when looking at the docs, there is no section in the method detail sendResetPasswordEmail where it discusses any errors it can throw, I assume it might also throw other general errors as well. In consideration of all of these, I decided to just make the code work for all types of errors. I can change it to only do FirebaseErrors though.

Actually now that I think about it, the error would still be surpressed anyway if we did just FirebaseErrors, so it would not be any difference except cleaner code. I'll do that sometime tomorrow.

@jtm13
Copy link
Contributor Author

jtm13 commented Jan 29, 2023

Made the state pass through the URL and call router.push to reload page

@jtm13 jtm13 linked an issue Jan 29, 2023 that may be closed by this pull request
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.

Reset Password needs to handle firebase error
2 participants