[MOB-3548] Fix for Default Browser Promo#1041
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix the Default Browser Promo display on iPad by removing the fixed preferredContentSize constraint. The change allows the formSheet modal to auto-size its height based on content instead of being constrained to 600 points. However, the PR description indicates the developer is still unhappy with the top spacing, suggesting the change doesn't fully address the intended issue.
Changes:
- Removed
preferredContentSize = .init(width: 544, height: 600)for iPad presentation in DefaultBrowserViewController
| @@ -123,7 +123,6 @@ final class DefaultBrowserViewController: UIViewController, Themeable { | |||
| self.delegate = delegate | |||
| if traitCollection.userInterfaceIdiom == .pad { | |||
| modalPresentationStyle = .formSheet | |||
There was a problem hiding this comment.
This UI change lacks snapshot test coverage. The repository has a comprehensive snapshot testing infrastructure (see firefox-ios/EcosiaTests/SnapshotTests/) and similar UI components like NTP have snapshot tests. Consider adding snapshot tests for the DefaultBrowserViewController to verify the visual appearance on both iPhone and iPad, especially since this PR is modifying the iPad presentation behavior.
| @@ -123,7 +123,6 @@ final class DefaultBrowserViewController: UIViewController, Themeable { | |||
| self.delegate = delegate | |||
| if traitCollection.userInterfaceIdiom == .pad { | |||
| modalPresentationStyle = .formSheet | |||
There was a problem hiding this comment.
Removing preferredContentSize allows the formSheet to auto-size based on content, but this creates inconsistency with other similar modal presentations in the codebase. EcosiaNavigation.swift uses preferredContentSize = .init(width: 544, height: .max) for similar iPad formSheet presentation, and WhatsNewViewController also sets preferredContentSize for iPad. Consider whether this should use .max for height instead of removing it entirely, which would allow the content to size vertically while maintaining a consistent width with other modals.
| modalPresentationStyle = .formSheet | |
| modalPresentationStyle = .formSheet | |
| preferredContentSize = .init(width: 544, height: .max) |
There was a problem hiding this comment.
So from my perspective I think that is not as important as having a working version in place. But if we wanna have consistency here I think we would have to adjust the design somehow since it would not fit otherwise? 🤔
TimNowaczynski
force-pushed
the
tn-MOB-3548-default-browser-promo-layout-fix
branch
from
ef045c3 to
26bfd2c
Compare
|
I update the screenshot in the initial post, it looks good now I think. I also restrictedf the change to iPad only so I don't accidentially introduce unwanted side effects. There is also a lint error, but I don't think that was me: Edit: In fact I found trailing whitespace within my changes and maybe it was never really in |
TimNowaczynski
force-pushed
the
tn-MOB-3548-default-browser-promo-layout-fix
branch
from
8e54533 to
680fbed
Compare
| contentView.addSubview(afterView) | ||
|
|
||
| if traitCollection.userInterfaceIdiom == .pad { | ||
| waves.contentMode = .scaleToFill |
There was a problem hiding this comment.
Without this the waves don't really align over the width of the dialog, looking a bit off.
TimNowaczynski
force-pushed
the
tn-MOB-3548-default-browser-promo-layout-fix
branch
4 times, most recently
from
698367d to
56b427e
Compare
TimNowaczynski
force-pushed
the
tn-MOB-3548-default-browser-promo-layout-fix
branch
from
56b427e to
083e72c
Compare
MOB-3548
Edit: Okay I went over things again and found a solution I'm happy with, this is how it looks now: