-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update localized.js #10432
base: develop
Are you sure you want to change the base?
Update localized.js #10432
Conversation
Country should be a combo field openstreetmap#7123 has been solved.
@@ -521,16 +521,52 @@ export function uiFieldLocalized(field, context) { | |||
loadCountryCode(); | |||
return localized; | |||
}; | |||
async function fetchCountryData() { | |||
const response = await fetch('https://restcountries.com/v3.1/all'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading a third-party resource on demand raises privacy concerns. Anyways, the list of countries doesn’t change often enough to require fetching a new list every time the user uses this field. In the worst case, we could hard-code the list of valid country codes, but we don’t have to do that either: we already depend on country-coder; the code you replaced refers to this package in order to get the ISO code of the country that the user is currently looking at. You can use this same package to enumerate all the valid country codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file and the localized
field type have nothing to do with the Country field. Rather, it’s for the Name field, which lets you choose from a list of languages, not countries. The reason it deals with country codes at all is that some language codes include country codes to indicate a dialect, for example, zh-CN
for Chinese in Hong Kong.
Contrary to the title of #7123, the Country field is already of type combo
, but the issue is that it doesn’t provide any suggestions other than what comes from taginfo by default. So we need to special-case the combo field type to do something different for the key country
. The place to do that is in modules/ui/fields/combo.js, which corresponds to that field type.
The point I made before is that, if we need to provide a similar list of countries in multiple fields, somewhere in modules/presets/index.js might be a good place to centrally calculate that list so it can be reused. But let’s start with just combo.js and the Country field specifically for now.
async function loadCountryCode() { | ||
var extent = combinedEntityExtent(); | ||
var countryCode = extent && countryCoder.iso1A2Code(extent.center()); | ||
_countryCode = await getISOCode(countryCode) || (countryCode && countryCode.toLowerCase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a reminder, the task is to display a list of countries by name and apply a country code in response to the user selecting a country name from the list. Intl.DisplayNames
is a useful API for converting country codes to names in the user’s language (only for display purposes).
Country should be a combo field #7123 has been solved. As mentioned in the issue " Since there are so many potential options to list for this field, we would want to generate them dynamically at runtime instead of hard-coding them in id-tagging-schema, but maybe not directly in the UI code, because other modules like the validator might eventually need the same data." This has been solved increasing the performance too.