-
Notifications
You must be signed in to change notification settings - Fork 112
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
Upgrade to support Xcode 14.0 #7062
Changes from 13 commits
5f86640
f22b616
5688454
73e0ff8
d5c1cdd
9f3a741
4318ef1
750f5f8
ea87d95
af1e24b
2a36b11
0405e14
92915ef
63e6e05
eebb6e8
21de10c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -31,6 +31,25 @@ final class HubMenuViewController: UIHostingController<HubMenu> { | |||||||||||||||||||||
func showPaymentsMenu() { | ||||||||||||||||||||||
show(InPersonPaymentsMenuViewController(), sender: self) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
override func viewWillAppear(_ animated: Bool) { | ||||||||||||||||||||||
super.viewWillAppear(animated) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// We want to hide navigation bar *only* on HubMenu screen. But on iOS 16, the `navigationBarHidden(true)` | ||||||||||||||||||||||
// modifier on `HubMenu` view hides the navigation bar for the whole navigation stack. | ||||||||||||||||||||||
// Here we manually hide or show navigation bar when entering or leaving the HubMenu screen. | ||||||||||||||||||||||
if #available(iOS 16.0, *) { | ||||||||||||||||||||||
self.navigationController?.setNavigationBarHidden(true, animated: animated) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
override func viewWillDisappear(_ animated: Bool) { | ||||||||||||||||||||||
super.viewWillDisappear(animated) | ||||||||||||||||||||||
|
||||||||||||||||||||||
if #available(iOS 16.0, *) { | ||||||||||||||||||||||
self.navigationController?.setNavigationBarHidden(false, animated: animated) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
Comment on lines
+47
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: does it make sense to reverse the order of the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API doc says
I don't think the order of these two statements would make any difference, the pop animation should begin and end at the same time. |
||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
private extension HubMenuViewController { | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,9 @@ struct ShippingLineDetails: View { | |
symbol: nil, | ||
keyboardType: .default) | ||
.accessibilityIdentifier("add-shipping-name-field") | ||
.onTapGesture { | ||
focusAmountInput = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you have the testing steps for this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was added so that the BTW, the issue was mentioned in my original PR.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, you can find my many attempts of fixing this issue in this buildkite branch builds. From the test result bundle (you can download one from the artifacts of this step), it appears even thought "add-shipping-name-field" is tapped, it's still "add-shipping-amount-field" that has the keyboard focus, which caused the test to fail. |
||
} | ||
} | ||
.padding(.horizontal, insets: safeAreaInsets) | ||
.addingTopAndBottomDividers() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -341,7 +341,11 @@ final class StoreStatsPeriodViewModelTests: XCTestCase { | |
viewModel.selectedIntervalIndex = 0 | ||
|
||
// Then | ||
XCTAssertEqual(timeRangeBarViewModels.map { $0.timeRangeText }, ["Monday, Jan 3", "Monday, Jan 3, 1:00 AM"]) | ||
if #available(iOS 16.0, *) { | ||
XCTAssertEqual(timeRangeBarViewModels.map { $0.timeRangeText }, ["Monday, Jan 3", "Monday, Jan 3 at 1:00 AM"]) | ||
} else { | ||
XCTAssertEqual(timeRangeBarViewModels.map { $0.timeRangeText }, ["Monday, Jan 3", "Monday, Jan 3, 1:00 AM"]) | ||
} | ||
Comment on lines
+344
to
+348
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice catch! |
||
} | ||
|
||
func test_timeRangeBarViewModel_for_thisWeek_is_emitted_twice_after_order_and_visitor_stats_updated_and_selecting_interval() { | ||
|
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.
do you still see the navigation bar issue #7091 with the latest Xcode 14? somehow I don't see this issue anymore on
trunk
using Xcode 14 in an iOS 16.0 simulator. I don't have an iOS 16 device though, might be worth testing on one to confirm if this workaround is still needed.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.
Just tried to comment out
viewWillAppear
andviewWillDisappear
- reproduced the issue on the device: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.
@jaclync Hmm, that's strange. I just run the trunk branch on an iPhone 14 simulator using Xcode 14, and verified this is still an issue.
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.
Sounds like something odd on my end then, thanks for checking both!