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 localized attributes for subscription period and introductory period to SKProduct #315

Closed

Conversation

funkenstrahlen
Copy link

  • Added support for introductory period and price to SKProduct (this is limited to iOS 11.2
  • Renamed the file as it is no longer limited to price
  • Added localized period function. This might need some improvement.

@bizz84
Copy link
Owner

bizz84 commented Dec 13, 2017

1 Error
🚫 Tests were not updated
2 Warnings
⚠️ Please target PRs to develop branch
⚠️ No CHANGELOG changes made

Generated by 🚫 Danger

@bizz84
Copy link
Owner

bizz84 commented Dec 16, 2017

@funkenstrahlen thank you for contributing, looks good.

I need to update the Travis settings to run Xcode 9.2 on CI - should be able to merge soon after.

@bizz84 bizz84 changed the base branch from master to develop December 16, 2017 14:38
case .month: return String(format: NSLocalizedString("localized subscription period month", comment: "localized subscription period month"), arguments: [period.numberOfUnits])
case .year: return String(format: NSLocalizedString("localized subscription period year", comment: "localized subscription period year"), arguments: [period.numberOfUnits])
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

This method introduces arbitrary localized strings which are not defined in a strings table.

I think it should be up to users to decide how to format these (in other words, this is application business logic).

In short, I believe it could be convenient to introduce localizedIntroductoryPrice as you have done, but I would leave localizedSubscriptionPeriod and localizedIntroductoryPeriod out of this PR.

I might borrow some of your code to calculate localizedIntroductoryPrice and make a separate PR to show my proposed approach.

@bizz84
Copy link
Owner

bizz84 commented Dec 16, 2017

Merged #318 which includes some of your changes.

Let me know about your thoughts on the localized period function.

@bizz84
Copy link
Owner

bizz84 commented Dec 17, 2017

@funkenstrahlen localizedIntroductoryPrice is now available on version 0.11.1.

I'm closing this PR for now, thanks again for your contribution and feel free to share your thoughts about localizedSubscriptionPeriod.

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