varunKT001 / tomper-wear-ecommerce

E-commerce web-application for selling clothing essentials 😀
https://tomper-wear.netlify.app
MIT License
38 stars 48 forks source link

create a centralized firebase auth error handler #58

Closed Devika-Sahu closed 1 year ago

Devika-Sahu commented 2 years ago

Issue reference: : #46

Proposed changes: create a centralized firebase error handler that handles firebase auth errors.

Type of change: : Bug fix

Please delete options that are not relevant.

Checklist:

Additional info (if any):

netlify[bot] commented 2 years ago

Deploy request for tomper-wear pending review.

Visit the deploys page to approve it

Name Link
Latest commit 6f55870da4dd7c8f5011ecec86662c1acdbab179
varunKT001 commented 2 years ago

@Devika-Sahu

You deleted the env.sample 🙂

You had to create a .env file and copy the contents of .env.sample to it, you just renamed it to .env 😅

Anyways fix that and I'll review it 👍

Devika-Sahu commented 2 years ago

@varunKT001 I have fixed that issue and committed it.

varunKT001 commented 2 years ago

@Devika-Sahu First of all, really sorry for being late, I had some urgent work 😞

I have reviewed your pull request. You have to make some more changes:

  1. You have not provided a default case in the error handler. There are more types of errors apart from the ones you made cases for. For example: image For this kind of error, you can use the default case with the default error message:
default:
  toast.error(`Error: {errorCode}`);
  break;
  1. Make the firebaseAuthErrorHandler as a default export:
export default firebaseAuthErrorHandler;
  1. Use prettier to format all the files you have modified. If you are using VS Code you can install the prettier extension and from VS Code settings, enable format on save. Also, for future pull requests, make sure you do this step 👍

Make these changes and I'll review it without any delay now 👍

Devika-Sahu commented 2 years ago

@varunKT001 Could you please merge my PR as it's the time of evaluation.

varunKT001 commented 2 years ago

@Devika-Sahu Make the required changes I mentioned. Once you make the changes I'll merge your PR.