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

Add configForLocale to PaywallData #1227

Merged
merged 2 commits into from
Sep 7, 2023
Merged

Add configForLocale to PaywallData #1227

merged 2 commits into from
Sep 7, 2023

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Sep 7, 2023

Counterpart of https://github.com/RevenueCat/purchases-ios/blob/paywalls/Sources/Paywalls/PaywallData.swift#L139

Tested passing Locale("en") and Locale.US

Tests will come in future PR where I add tests for PaywallData

@vegaro vegaro added feat A new feature RevenueCatUI labels Sep 7, 2023
@vegaro vegaro requested a review from a team September 7, 2023 10:22
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Just a comment but looks good!

*/
fun configForLocale(requiredLocale: Locale): LocalizedConfiguration? {
return localization[requiredLocale.toString()]
?: localization.takeIf { (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) }?.let { localization ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Since paywalls like likely have this or higher minVersion, I think this makes sense 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh actually! I thiknk this is not necessary and it's from previous code that was using languageTag, let me check

return localization[requiredLocale.toString()]
?: localization.takeIf { (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) }?.let { localization ->
localization.entries.firstOrNull { (localeKey, _) ->
Locale(localeKey).isO3Language == requiredLocale.isO3Language
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm is this a typo? Should it be iso3Language? If not, I wonder if we should use the get method, since this looks weird...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha I know it looks weird, but it's not a typo. I will use the get method yes

@vegaro vegaro merged commit 408b404 into paywalls Sep 7, 2023
3 of 5 checks passed
@vegaro vegaro deleted the add_config_for_locale branch September 7, 2023 11:02
*
* @return [LocalizedConfiguration] for the given [Locale], if found.
*/
fun configForLocale(requiredLocale: Locale): LocalizedConfiguration? {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏻

I'm guessing tests will come later? 😇

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

Successfully merging this pull request may close these issues.

None yet

3 participants