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

Fixed-Phone-Number-Validation #17

Closed bhavya-parikh closed 2 years ago

bhavya-parikh commented 2 years ago

Issue reference: https://github.com/varunKT001/tomper-wear-ecommerce/issues/3 Form was taking any number of digits and was allowing digits even more than 10,

Proposed changes:

Added validation check for phone number

Type of change:

Please delete options that are not relevant.

Checklist:

Additional info (if any):

netlify[bot] commented 2 years ago

🔮 Deploy Preview for tomper-wear canceled.

🔨 Explore the source changes: 9e604bd91b722dc195b463f3934b5227a8d5bc6d

🔍 Inspect the deploy log: https://app.netlify.com/sites/tomper-wear/deploys/622072f85aa6c20007d44c66

varunKT001 commented 2 years ago

Hey @Bhavya-Parikh 👋, First of all, to fill out the checklist, you have to do this:

Do this 
- [x]

Not this
- [✔]

Secondly, the way you have written the regex is not correct. It won't work. This is the correct way:

if (!phone_number || !/^\d{10}$/.test(phone_number) {
    ...
}

You have changed the error message to Enter valid phone number. It was not to be changed. Please don't make any changes to it.

Also, please write a good commit message like updated phone number validation.

Now since you have already committed your change, it would require a force push to the main branch to rename the message, which is not good actually. I can squash and merge to avoid this, but it's not a good practice. So go ahead, delete your fork, fork it again, make the changes again and make a new pull request with a good commit message this time.

I am closing this pull request as of now.