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

feat: Settings country dependencies #310

Merged
merged 9 commits into from
Feb 27, 2024
Merged

Conversation

dq18
Copy link
Collaborator

@dq18 dq18 commented Feb 9, 2024

What

Thanks to #297, it is now possible to select a country.

  • The Settings page has been updated to change the currency to the country currency automatically (it can still be changed by the user)
  • The Settings page will also list the current language and the country languages at the top of the dropdown list
  • Minor fixes

@dq18 dq18 requested a review from raphodn February 9, 2024 00:10
@dq18 dq18 changed the base branch from master to user_country February 15, 2024 11:23
@dq18 dq18 force-pushed the settings_country_dependancies branch from b6a5358 to 30977eb Compare February 15, 2024 11:38
@dq18 dq18 changed the title Feat: Settings country dependencies feat: Settings country dependencies Feb 15, 2024
@dq18 dq18 force-pushed the settings_country_dependancies branch from 30977eb to b2c18f9 Compare February 16, 2024 17:16
Base automatically changed from user_country to master February 16, 2024 17:39
@@ -94,7 +94,34 @@ export default {
this.languageTranslationCompletion = await localeManager.calculateTranslationCompletion(this.userSettingsForm.selectedLanguage.code)
}
},

'userSettingsForm.selectedCountry': function (newValue, oldValue) {
Copy link
Member

@raphodn raphodn Feb 21, 2024

Choose a reason for hiding this comment

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

i just tested. it works, but it feels kinda strange that when changing the country, both the language & currency are changed without the user realizing 🤔

  • we should have a short message popup (like when uploading a proof)
  • or separate the behavior and not automatically make the changes.. much simpler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Language will not be automatically changed, the current selected language (or browser language or default English) will still be the one selected. But in the dropdown it will show up first, followed by languages of the country and then other languages.
I suppose the same can be done for currency. Or have a popup. But ultimately we will change the currency dropdown to a checkbox list for the user to choose currencies that he is paying with (issue #359 )
I will leave it that way for now and do another PR for currency checkbox dropdown.

Copy link
Member

Choose a reason for hiding this comment

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

I tested again and the currency changes. can we have a small snackbar to let the user know ?

also the code below seems very complex... I'm not sure we need to go into all these automation, vs just let users choose a country / a language / a currency 🤔

Copy link
Member

@raphodn raphodn Feb 22, 2024

Choose a reason for hiding this comment

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

it makes sense on app startup when we don't know anything about the user, and we can guess as best we can these 3 info.

but in the settings page it would be much simpler to have 3 dropdowns = 3 simple choices..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I will update #372 to take this into consideration and revert to decoupling of currency.

if (selectedCountry && selectedCountry.currency && selectedCountry.currency.length > 0) {
this.userSettingsForm.currency = selectedCountry.currency[0]
} else {
this.userSettingsForm.currency = 'USD'
Copy link
Member

Choose a reason for hiding this comment

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

why is USD hardcoded here ? we have EUR set as default in the store. we could move everything to an env btw and have a VITE_DEFAULT_CURRENCY (like LOCALE & COUNTRY)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I used that for testing and forgot to remove. The default should revert to VITE_DEFAULT_CURRENCY set in the store and the "else" statement is useless here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wasn't for testing. I changed to the store value

@dq18 dq18 merged commit c52f046 into master Feb 27, 2024
2 checks passed
@dq18 dq18 deleted the settings_country_dependancies branch February 27, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants