-
-
Notifications
You must be signed in to change notification settings - Fork 37
Show purchase status in settings #426
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
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request refactors the settings view model's about section element initialization to explicitly type cell view models and restructure subscription-related conditional logic. It replaces generic button initializers with typed cell view models, introduces a new status cell for displaying full version unlock status with a checkmark, and adjusts the branching conditions that determine which UI elements appear based on subscription and full version unlock states. A new localization string is added for the full version status label. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cryptomator/Settings/SettingsViewModel.swift(1 hunks)SharedResources/en.lproj/Localizable.strings(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Cryptomator/Settings/SettingsViewModel.swift (5)
Cryptomator/Common/Cells/ButtonCellViewModel.swift (1)
createDisclosureButton(19-21)Cryptomator/Settings/SettingsCoordinator.swift (3)
showAbout(31-35)showManageSubscriptions(85-97)showUnlockFullVersion(79-83)CryptomatorCommon/Sources/CryptomatorCommonCore/LocalizedString.swift (1)
getValue(12-22)Cryptomator/Purchase/IAPViewController.swift (1)
restorePurchase(277-290)Cryptomator/Purchase/PurchaseViewController.swift (1)
restorePurchase(55-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test (freemium)
🔇 Additional comments (2)
SharedResources/en.lproj/Localizable.strings (1)
216-216: LGTM! Localization key follows conventions and uses consistent terminology.The new localization string is well-formed and appropriately placed within the Settings section. The key name "fullVersionStatus" clearly indicates its purpose, and "Full Version" matches the terminology used throughout the app.
Cryptomator/Settings/SettingsViewModel.swift (1)
79-84: Status cell implementation is correct.The localization key
"settings.fullVersionStatus"exists in the English localization file with the value "Full Version". The cell implementation is appropriate, usingBindableTableViewCellViewModelwithselectionStyle: .noneandaccessoryType: .checkmarkto correctly represent a non-interactive status indicator.
| elements.append(statusCell) | ||
| } else { | ||
| elements.append(ButtonCellViewModel.createDisclosureButton(action: SettingsButtonAction.showUnlockFullVersion, title: LocalizedString.getValue("settings.unlockFullVersion"))) | ||
| elements.append(ButtonCellViewModel<SettingsButtonAction>(action: .restorePurchase, title: LocalizedString.getValue("purchase.restorePurchase.button"))) |
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 never really paid attention to this "restore purchase" button before, but I think we should remove it completely from this screen. At the moment, it doesn't give the user any useful feedback. In the purchase screen, it works differently and shows some alert.
Before your change, it was only visible when there was a running subscription. I still don't know why this would be necessary in that case. Or if removing this button feels too risky, revert it to how it was before.
| let statusCell = BindableTableViewCellViewModel( | ||
| title: LocalizedString.getValue("settings.fullVersionStatus"), | ||
| selectionStyle: .none, | ||
| accessoryType: .checkmark | ||
| ) | ||
| elements.append(statusCell) |
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.
Let's think about an alternative here. At the moment, it looks like some "boolean" setting that you can turn on or off. But not only that, it also looks like a button that you can tap.
My suggestion is to not use a table cell at all. How about a footer for this section?
Adds a visual indicator for users with a lifetime license in the Settings screen. Users with an active lifetime purchase now see a "Full Version" label with a checkmark, confirming their premium status. Additionally, the "Restore Purchase" button has been removed for subscription users as they already have active premium access.