Skip to content

Conversation

fadi-george
Copy link
Contributor

@fadi-george fadi-george commented Sep 22, 2025

Description

1 Line Summary

Follows guide: https://intl-tel-input.com/examples/lookup-country.html for auto-detecting country code.

Details

  • updates window.intlTelInput config to set initial country to "auto"
  • adds a getCountryCode method that fetches country code enough from an api with fallback apis if they error

Systems Affected

  • WebSDK
  • Backend
  • Dashboard

Validation

Tests

Info

Checklist

  • All the automated tests pass or I explained why that is not possible
  • I have personally tested this on my machine or explained why that is not possible
  • I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

  • Don't use default export
  • New interfaces are in model files

Functions:

  • Don't use default export
  • All function signatures have return types
  • Helpers should not access any data but rather be given the data to operate on.

Typescript:

  • No Typescript warnings
  • Avoid silencing null/undefined warnings with the exclamation point

Other:

  • Iteration: refrain from using elem of array syntax. Prefer forEach or use map
  • Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Info

Checklist

  • I have included screenshots/recordings of the intended results or explained why they are not needed

Related Tickets



This change is Reviewable

@fadi-george fadi-george force-pushed the fadi/sdk-2965-web-sdk-sms-auto-lookup branch from b315053 to c14bf39 Compare September 22, 2025 23:49
@fadi-george fadi-george force-pushed the fadi/sdk-2965-web-sdk-sms-auto-lookup branch from c14bf39 to 683264b Compare September 22, 2025 23:50
@jkasten2
Copy link
Member

jkasten2 commented Sep 23, 2025

@fadi-george can we not use new Intl.Locale(navigator.language).region instead?

I have two concerns about using 3rd party servers for IP to geo location:

  1. Meeting the term and conditions of those vendors
  2. Impact to customers and end-users of making external calls to another service.

@fadi-george
Copy link
Contributor Author

fadi-george commented Sep 23, 2025

@fadi-george can we not use new Intl.Locale(navigator.language).region instead?

I have two concerns about using 3rd party servers for IP to geo location:

  1. Meeting the term and conditions of those vendors
  2. Impact to customers and end-users of making external calls to another service.
  1. With Chrome you can set preferred language to be languages like Arabic , English, etc. These are pretty generic and wont set a region e.g.
new Intl.Locale('en').region 
undefined
new Intl.Locale('en-US').region 
'US'

navigator.languages
(3) ['en-US', 'en', 'sl']
  1. If the request errors, it'll just default to 'us' country code. It would only apply for new/subscribed users.
Screenshot 2025-09-23 at 1 53 43 PM

@jkasten2
Copy link
Member

jkasten2 commented Sep 23, 2025

With navigator.language / navigator.languages I see what you are saying, there are a number of cases where this value isn't going have a county set, it depends on the browser having a language set with a region subtag. On my browsers (Chrome and Firefox) had navigator.language set to en-US.

However if you live in a country where that language isn't a top language used then it won't be on the browser's language list, or even if there is a matching combination there isn't guarantee it gets set.

  • Example I see Spanish has a US option in Chrome, but not Firefox.

An IP address would probably be more accurate for this Phone Number input in more cases, but there a few cases were it would not too; VPN or those traveling to another country.
If there were more things depended on getting country correct than it might be worth looking into the 3rd party questions I have, noted in my first comment, but I don't think it is worth going down that road for one small feature.

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.

2 participants