Add ability to toggle edit mode for vertical mode #3576
Conversation
|
The following translations are missing in Lokalise. While they don't need to be downloaded into the codebase as part of this PR, they do need to exist in Lokalise so that they can be downloaded as part of the release process. Manage payment methods By adding the label |
...llerSnapshotTests/test_VerticalSavedPaymentOptionsViewControllerSnapshotTestsDarkMode@3x.png
Outdated
Show resolved
Hide resolved
| navBar.additionalButton.setTitle(UIButton.editButtonTitle, for: .normal) | ||
| navBar.additionalButton.setTitleColor(configuration.appearance.colors.primary, for: .normal) | ||
| navBar.additionalButton.setTitleColor(configuration.appearance.colors.primary.disabledColor, for: .disabled) | ||
| navBar.additionalButton.accessibilityIdentifier = "edit_saved_button" | ||
| navBar.additionalButton.titleLabel?.adjustsFontForContentSizeCategory = true | ||
| navBar.additionalButton.addTarget(self, action: #selector(didSelectEditSavedPaymentMethodsButton), for: .touchUpInside) |
There was a problem hiding this comment.
I believe this is the 4th place we perform similar configuration to create an Edit button (search for "edit_saved_button"). Feels like an opportunity to DRY.
| private func paymentMethod(from button: PaymentMethodRowButton) -> STPPaymentMethod? { | ||
| return paymentMethodRows.first(where: { $0.button === button })?.paymentMethod | ||
| } |
There was a problem hiding this comment.
Picking up this convo #3573 (comment)
I understand your point but IMO it's worth making this payment method a property of PaymentMethodRowButton, it lets you get rid of this funny search and removes the possibility that the STPPaymentMethod is nil.
There was a problem hiding this comment.
👍, refactored out the ViewModel and instead inject a STPPaymentMethod.
|
|
||
| // Disable interaction to prevent double selecting since we will be dismissing soon | ||
| self.view.isUserInteractionEnabled = false | ||
| self.navigationBar.isUserInteractionEnabled = false // Tint buttons in the nav bar to look disabled |
There was a problem hiding this comment.
Not sure I understand this comment, is it supposed to be a TODO?
There was a problem hiding this comment.
Ah, maybe a bad/un-needed comment but the main rational behind disabling the interaction of the nav bar is to give the disabled look to the buttons in the nav bar.
Right after selecting:
There was a problem hiding this comment.
Isn't the main rationale to disable interaction? You wouldn't want to be able to edit while we are in the middle of transitioning back to the main screen.
| @@ -132,16 +132,19 @@ class SheetNavigationBar: UIView { | |||
| // MARK: - | |||
| enum Style { | |||
| case close(showAdditionalButton: Bool) | |||
| case back | |||
| case back(showAdditionalButton: Bool) | |||
There was a problem hiding this comment.
Huh, why is showing the additional button tied to the style? Looks like I wrote this code but I don't recall. Does it make sense? Should showAdditionalButton be a separate bool, independent of the style enum?
There was a problem hiding this comment.
I don't feel too strongly about this, I see this more as a convenience to toggle the additional button when switching styles, instead of switching the style then hiding/showing the additional button if needed
There was a problem hiding this comment.
For convenience we could still have a update(style: Style, showAdditionalButton: Bool) method. But it's very minor so I'm fine to leave it alone.
| testWindow.rootViewController = bottomSheet | ||
| STPSnapshotVerifyView(bottomSheet.view) |
There was a problem hiding this comment.
The screenshots are all using the test window's fixed height of 500 points @ 3x = 1500 pixels, we should fix so that it autosizes to the real height.
There was a problem hiding this comment.
Updated this so the window size is the height of the VC + the BottomSheet. For some reason even w/ a content view controller the height of the BottomSheet is always just the height of the nav bar (42). FWIW, the testing framework uses the screen bounds as the VC bounds when generating it's view, but instead we use FBSnapshotVerifyView.
There was a problem hiding this comment.
Hmm weird. We should have one simple way to snapshot test these bottom sheet content VCs, we're doing this many times and shouldn't be re-solving it each time.
Our content VCs are special in that they are not presented full screen like normal VCs so I think it's important to simulate using the height of the VC as it would appear in the bottom sheet. Like if our VC has a bug that causes it to be too tall when presented in the sheet, that would be good to catch in the snapshot test.
There was a problem hiding this comment.
Agreed, I'll look at this closer in a following PR
Summary
Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-05-12.at.13.54.45.mp4
Motivation
https://jira.corp.stripe.com/browse/MOBILESDK-2026
Testing
Changelog
N/A