Skip to content

Conversation

arisGio
Copy link
Contributor

@arisGio arisGio commented Feb 9, 2025

  • area code resource
  • Hey @tsevdos!
  • I am still unable to assign myself when creating a pr for some reason.
  • Could you please also create an issue for this feature too?

Copy link

changeset-bot bot commented Feb 9, 2025

⚠️ No Changeset found

Latest commit: 5fafe4e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@arisGio arisGio marked this pull request as ready for review February 9, 2025 21:13
@tsevdos tsevdos linked an issue Feb 10, 2025 that may be closed by this pull request
@tsevdos
Copy link
Owner

tsevdos commented Feb 10, 2025

Hello there @arisGio ! Thanks once again for the contribution!!!! I have created an issue but I also cannot assign you on it!!!! I'll have to check this article and hopefully will find a way to fix this...

In the mean time I'll have a small suggestion (for consistency - I'll leave it as a code review).

@tsevdos tsevdos self-requested a review February 10, 2025 10:28
@tsevdos tsevdos added the enhancement New feature or request label Feb 10, 2025
Comment on lines 113 to 114
*/
export function isValidMobilePhone(mobilePhone: string): boolean {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we also support numbers as well (for this and all the other phone related fns)? It is not a big change, just a minor check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

However, when updating the test cases I saw that numbers starting with 00 are not acceptable by the linter so did not add them to the tests.

Comment on lines 115 to 117
const mobilePhoneRegex = RegExp(/^(\+30|0030)?69\d{8}$/);

return mobilePhoneRegex.test(mobilePhone.replace(/[\s\-().]/g, ""));
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep it as simple as possible.

Suggested change
const mobilePhoneRegex = RegExp(/^(\+30|0030)?69\d{8}$/);
return mobilePhoneRegex.test(mobilePhone.replace(/[\s\-().]/g, ""));
let mobilePhoneStr = mobilePhone;
if (typeof mobilePhone === "number") {
mobilePhoneStr = String(num).slice(-10)
}
const mobilePhoneRegex = RegExp(/^(\+30|0030)?69\d{8}$/);
return mobilePhoneRegex.test(mobilePhoneStr.replace(/[\s\-().]/g, ""));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @tsevdos!

Numbers starting with 2 zeros are not accepted at all.

Since either way we use string type to apply logic for each method then I decided first things first to convert to string using the respective constructor.

Removed any RegExp constructors to simplify code even further and kept it only in the case of any template literals.

@arisGio arisGio requested a review from tsevdos February 18, 2025 19:21
Copy link

@tsevdos tsevdos merged commit 7d82bb6 into tsevdos:main Feb 22, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Development

Successfully merging this pull request may close these issues.

Add (greek) phone validations

2 participants