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

ENH: Phone Input Component #189

Merged
merged 12 commits into from
Sep 12, 2023

Conversation

Ahtritus
Copy link
Contributor

@Ahtritus Ahtritus commented Sep 6, 2023

image
image
image

Hey @JhumanJ, made some more changes, do take a look now. Hope this is the expected behavior.

/claim #170

@Ahtritus
Copy link
Contributor Author

Ahtritus commented Sep 8, 2023

@JhumanJ Hey, were you able to take a look at this one yet?

Copy link
Owner

@JhumanJ JhumanJ left a comment

Choose a reason for hiding this comment

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

Hey, it looks better but please give your PR a try:

  • It misses the package in composer.json
  • It misses the data files for the list of countries

resources/js/app.js Outdated Show resolved Hide resolved
resources/js/app.js Outdated Show resolved Hide resolved
resources/js/components/forms/PhoneInput.vue Show resolved Hide resolved
resources/js/components/forms/index.js Show resolved Hide resolved
},
watch: {
inputVal(newVal, oldVal) {
this.compVal = this.selectedCountryCode.dial_code + newVal;
Copy link
Owner

Choose a reason for hiding this comment

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

Please make sure to read all requirements

It should handle clashes for instance, for French number you don't need +33 and start with a 0, so if user selects +33 for France, and enters 0123456789, the resulting value should not be +330123456789 but +330123456789

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I'm reading it right but do you mean that the result should be +33123456789 instead of +330123456789, with the extra 0 ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added the new logic to handle this scenario, please take a look

Copy link
Owner

@JhumanJ JhumanJ left a comment

Choose a reason for hiding this comment

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

Great progress, almost there. Please check the review comments, and make sure to read all requirements from #170. Thanks for the great work!

app/Rules/ValidPhoneInputRule.php Outdated Show resolved Hide resolved
resources/js/components/forms/PhoneInput.vue Outdated Show resolved Hide resolved
</script>

<style scoped>
.dropdown {
Copy link
Owner

Choose a reason for hiding this comment

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

Please stick to tailwind. We avoid css as much as possible, and usually don't use actual classes. Ex:

  • Remove the class dropdown and replace it with relative w-1/4

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, using tailwind classes. Also, added a prop to the VSelect component as I wanted the opened dropdown options to have greater width than the selected option when dropdown closed. Do take a look at that

@Ahtritus
Copy link
Contributor Author

image

Currently this

@JhumanJ JhumanJ merged commit a53677d into JhumanJ:main Sep 12, 2023
1 check passed
@JhumanJ
Copy link
Owner

JhumanJ commented Sep 12, 2023

Great job, thank you @Ahtritus !

@Ahtritus
Copy link
Contributor Author

/claim #170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants