-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: add a country selector in the user settings #297
Conversation
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.
how did you generate this countries.json
? can you document it in a seperate README.md ? (in case we need to update it, new countries can appear ^^)
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.
The list comes from the same source as the one for languages:
https://github.com/annexare/Countries/blob/main/packages/countries/src/data/countries.ts
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.
if there is no changes compared to this library, why not add it directly to the repo ?
npm install countries-list
^^
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.
I removed some info that is not relevant from the countries list, like "Continent" and "Phone", and we will likely change the information in the future. The languages list through this library would also need a lot of workaround since locale codes are missing and it is not handling locales with country specificity like zh-TW
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.
ok we'll still need a script to explain/generate/update this country list somehow (like in #338 )
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.
"native": "Australia", | ||
"capital": "Canberra", | ||
"currency": ["AUD"], | ||
"languages": ["en"]}, |
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.
en_AU
instead ?
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.
We should keep it like this here. If we later want to use the country/language relationship in our app, I would add some logic to:
- check if a locale with country code exists
- if not then get the locale without country code
I am going to use this logic to display the country languages first in the language dropdown in a future PR
}, | ||
async updateSettings() { | ||
await localeManager.changeLanguage(this.userSettingsForm.selectedLanguage.code) | ||
this.appStore.setLanguage(this.userSettingsForm.selectedLanguage.code) | ||
this.appStore.setCountry(this.userSettingsForm.selectedCountry) |
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.appStore.setCountry(this.userSettingsForm.selectedCountry) | |
this.appStore.setCountry(this.userSettingsForm.selectedCountry.code) |
src/views/UserSettings.vue
Outdated
@@ -9,6 +9,15 @@ | |||
<v-card :title="$t('UserSettings.ChangeLanguage')" prepend-icon="mdi-earth"> | |||
<v-divider></v-divider> | |||
<v-card-text> | |||
<v-autocomplete | |||
v-model="userSettingsForm.selectedCountry" | |||
label="Country" |
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.
translation missing :)
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.
Thanks for seeing this. Corrected. Translation key is already in locale files (from a previous PR)
src/store.js
Outdated
@@ -10,6 +10,8 @@ export const useAppStore = defineStore('app', { | |||
last_currency_used: 'EUR', // TODO: init with user locale ? | |||
recent_locations: [], | |||
language: localStorage.getItem('user-locale') || import.meta.env.VITE_DEFAULT_LOCALE, // 'en' | |||
country: null, // will be of type Object like |
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.
shouldn't we init with 'FR' (instead of having a ||
in UserSettings) ?
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.
Definitely, corrected. 'FR' is initialized in store
src/views/UserSettings.vue
Outdated
v-model="userSettingsForm.selectedCountry" | ||
label="Country" | ||
:items="countryList" | ||
item-title="name" |
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.
item-title="name" | |
item-title="native" |
like the languages ?
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.
Corrected
What
In the user settings, add a country selector
Why ?
Screenshot