chore: support for macOS Designed for iPad mode - WPB-25053#4636
Conversation
Test Results1 865 tests 1 838 ✅ 2m 1s ⏱️ Results for commit 1bd54cc. ♻️ This comment has been updated with latest results. Summary: workflow run #26024622037 |
There was a problem hiding this comment.
Btw, requestAuthorization(_:) is deprecated.
| PHPhotoLibrary.requestAuthorization(for: .readWrite) { status in |
| <array/> | ||
| <key>UIRequiresFullScreen</key> | ||
| <true/> | ||
| <false/> |
There was a problem hiding this comment.
This is a big change. We need to test resizing thoroughly, especially with horizontalSizeClass changes before disabling this.
There was a problem hiding this comment.
Also any time we are doing UIDevice.current.userInterfaceIdiom we might be making assumptions about screen sizes.
There was a problem hiding this comment.
I did run it quickly and did not see any issue, do you have something in mind ?
There was a problem hiding this comment.
I'm not really sure but off the top of my head I guess the tab bar needs checking when in narrow split screen and possible edge insets for the bubbles in conversations.
samwyndham
left a comment
samwyndham
left a comment
There was a problem hiding this comment.
Nice work. I really like the idea of getting this working and it's nice that the changes aren't too great.
My main concern is that we are changing a few fundamentals in production. Some questions:
- Will Designed for iPad also appear in production?
- Can we make the change to full screen mode internal/beta only?
| SUPPORTED_PLATFORMS = "iphoneos iphonesimulator"; | ||
| SUPPORTS_MACCATALYST = NO; | ||
| SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD = NO; | ||
| SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD = YES; |
There was a problem hiding this comment.
question: Will this change apply to the production apps too?
There was a problem hiding this comment.
In my opinion we shouldn't do that. Users already have access to a mac app. At least this seems like a product decision.
| <key>UIRequiredDeviceCapabilities</key> | ||
| <array/> | ||
| <key>UIRequiresFullScreen</key> | ||
| <true/> |
There was a problem hiding this comment.
question: This is a pretty big change but probably the right direction to go. But have you thought about possible issues here? I guess there must be a bunch of things to test.
There was a problem hiding this comment.
Can we make this change only in non production builds?
There was a problem hiding this comment.
maybe not, or using a value from xcconfig
There was a problem hiding this comment.
I think we could remove UIRequiresFullScreen from Info.plist and set the value as build setting:
https://developer.apple.com/documentation/Xcode/build-settings-reference#Requires-Full-Screen
Than we're able to set different values (for AppStore and for internal TestFlight builds).
| <array/> | ||
| <key>UIRequiresFullScreen</key> | ||
| <true/> | ||
| <false/> |
There was a problem hiding this comment.
Also any time we are doing UIDevice.current.userInterfaceIdiom we might be making assumptions about screen sizes.
|
samwyndham
left a comment
samwyndham
left a comment
There was a problem hiding this comment.
@netbe This is cool and I'm close to approving but I don't think it should be enabled for production apps or at least the ramifications of this should be considered and discussed with product. The ramifications are:
- In the mac app store there would now be multiple versions of the app.
- We would have to support this new version and any new bugs
3 participants
Issue
In order to use more the Beta builds, this PR improve support for the app running on macOS via iPad design.
Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: