use iPhone metrics when the iPad app is running in compact width (closes #128) #129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chucks
commented
Aug 17, 2021
Sources/Styleguide/Padding.swift
Outdated
| ?? .grid(20) | ||
| ) | ||
|
|
||
| if self.deviceState.isUsingPadMetrics { |
There was a problem hiding this comment.
have a couple places here where I need to clean up indentation to be two-space
|
Thanks for submitting this! We haven't had a chance to look into it yet, but will try to soon! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the majority of the code that is checking for iPad UI idiom to code that checks for "effectively using iPad metrics", which I've defined as "iPad idiom and horizontal size class not compact". With this change in place, the app will select UI element sizes and layout values (e.g. padding) that are intended for iPhone when running on iPad in compact width.
This fix contains a shortcut that is probably fine for an app such as isowords, but is incorrect in the general case. Namely, the horizontal size class is tracked in the
DeviceStatestruct, which captures the horizontal size class high up in the view hierarchy, whereas it is OK for intermediate views to manually override this value. Thus, thehorizontalSizeClassvalue in the SwiftUI Environment on the view where you want to apply the metrics is the real "truth" and could diverge from what is in DeviceState. However, that makes the code a lot messier since it introduces another environment property into each of these views, plus each test about what metrics to apply becomesidiom == pad && horizontalSizeClass != compact.This change affects metrics and layout in the broader app, it's possible I've introduced some visual regressions since I've not tested all the flows extensively.
I have noticed some visual glitching when doing some rotation and the app is in compact width. I'm not certain if this is a regression or some artifact of running in the simulator. I was not successful getting the code signing to be generic enough to run a local build of the app on my devices.
If this problem is present when running on device, I don't mind working on it a little further if you have hints about where I should look.
An alternative "fix" for this problem would be to enable "requires full screen" in info.plist, which would stop supporting the multitasking scenarios completely.