add largeTitleDisplayMode to PaymentContext #849
Conversation
Stripe/STPPaymentContext.m
Outdated
|
|
||
| UINavigationController *navigationController = [[UINavigationController alloc] initWithRootViewController:paymentMethodsViewController]; | ||
| navigationController.navigationBar.stp_theme = self.theme; | ||
| navigationController.navigationBar.prefersLargeTitles = YES; |
There was a problem hiding this comment.
Based on the docs, it looks like it isn't necessary to expose this property on PaymentContext too – just setting it to always YES always seems fine.
When this property is set to true, the navigation bar allows the title to be displayed out-of-line and using a larger font. The navigation item used to build the bar must specify whether it wants its title displayed in the large or small format. Use the largeTitleDisplayMode property to configure the title's appearance.
When the property is set to false, the navigation bar displays the title inline with the other bar button items.
| @@ -35,6 +35,7 @@ class BrowseProductsViewController: UITableViewController { | |||
| super.viewDidLoad() | |||
| self.navigationItem.title = "Emoji Apparel" | |||
| self.navigationController?.navigationBar.isTranslucent = false | |||
| self.navigationController?.view.backgroundColor = .white | |||
There was a problem hiding this comment.
This fixes the transition when PaymentContext pushes a small title view controller onto a large title nav controller (there's a black flash without this line).
There was a problem hiding this comment.
Looks good, some small feedback
-
I think you might want to flesh out the documentation of the new method a bit more (i put some suggestions as a comment there).
-
Do we need to update our tests to cover this? Either regular unit tests or snapshot tests? (I made a bunch of snapshot test updates in the xcode9 branch if you want to wait for that to be merged).
| which causes the title to use the same styling as the previously displayed | ||
| navigation item. | ||
| */ | ||
| @property (nonatomic, assign) UINavigationItemLargeTitleDisplayMode largeTitleDisplayMode; |
There was a problem hiding this comment.
Might want to consider adding in some extra notes about how this property interacts with push vs present. Eg. the default of automatic won't work for present because there is no previous item. And for push we don't alter the owning navigation bar, you have to make sure it's navigation bar has prefersLargeTitles = YES in order for this to do anything.
There was a problem hiding this comment.
ah, good call – I'll expand
There was a problem hiding this comment.
Oh, you also are getting yelled at by travis because Xcode8 doesn't know about large title display mode. Which also makes me realize: You need to guard all these calls to it in availability checks and mark the property in the header as available ios 11+ only.
Will need to wait until #851 is merged and then rebase.
Ah yeah, now's probably a good time to add some PaymentContext snapshot tests. I'll wait and update the PR after #851 gets in |
bg-stripe
force-pushed
the
bg-large-title
branch
from
a6eaa89 to
e9dc0b4
Compare
|
I've updated docs, added availability guards, and added some snapshot tests (b10520a) for PaymentContext pushing payment methods / shipping onto a hostViewController. re-r? @bdorfman-stripe |
| @@ -9,5 +9,18 @@ | |||
| #import <FBSnapshotTestCase/FBSnapshotTestCase.h> | |||
|
|
|||
| @interface FBSnapshotTestCase (STPViewControllerLoading) | |||
| - (UIView *)stp_preparedAndSizedViewForSnapshotTestFromViewController:(UIViewController *)viewController; | |||
There was a problem hiding this comment.
Might be nice to retain this, or a similarly named method, that encompasses both the other helpers (since everything except Pay Context immediately calls both in a row I think?)
There was a problem hiding this comment.
ah yeah, that's a good call, I'll update
bg-stripe
force-pushed
the
bg-large-title
branch
from
b10520a to
da29424
Compare
* Add support for customizing button borders * Switch 2FA view to use the custom button * Cleanup * Updated 2FA view snapshots * Cleanup * Tweak plain button + add snapshot tests * Add tests for `linkPlain()` button * Fix typo * Handle dynamic colors * Fix tests * Fix test file name * Remove special character from file name Allows the test to be discovered and skipped.
r? @bdorfman-stripe
PaymentContextwill set any pushed or presented VC'slargeTitleDisplayModeto its own.re: #830 – I wasn't able to reproduce the layout issue, but this does fix not being able to set
largeTitleDisplayModeof pushed PaymentContext view controllers tonever.