Remove BottomSheetViewController usages – #1#23448
Conversation
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23448-6c9bb90 | |
| Version | 25.2 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 6c9bb90 | |
| App Center Build | WPiOS - One-Offs #10409 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23448-6c9bb90 | |
| Version | 25.2 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 6c9bb90 | |
| App Center Build | jetpack-installable-builds #9451 |
| extension CommentDetailInfoViewController { | ||
| func show(from presentingViewController: UIViewController, sourceView: UIView) { | ||
| let navigationController = UINavigationController(rootViewController: self) | ||
| if UIDevice.isPad() { |
There was a problem hiding this comment.
If you put the app on iPad into split screen, this branch will be executed and the view controller will be presented as a full screen modal instead of popover. Maybe we should check horizontal size class instead?
There was a problem hiding this comment.
Fixed. I changed it to presentingViewController.traitCollection.horizontalSizeClass == .regular.
Btw, the Split View is pretty much completely broken on iPad as you can't access any of your site menus.
| } else { | ||
| self.navigationItem.leftBarButtonItem = UIBarButtonItem(title: SharedStrings.Button.close, primaryAction: UIAction { [weak presentingViewController] _ in | ||
| presentingViewController?.dismiss(animated: true) | ||
| }) |
There was a problem hiding this comment.
I feel like it makes better sense to put this code into the if let sheet block below.
There was a problem hiding this comment.
If it's presented modally with the default settings, it still needs a "Cancel" button.
There was a problem hiding this comment.
Ah of course. I misread the code...
| sheet.detents = [.medium(), .large()] | ||
| sheet.prefersGrabberVisible = true | ||
| navigationController.additionalSafeAreaInsets = UIEdgeInsets(top: 8, left: 0, bottom: 0, right: 0) // For grabber | ||
| } |
There was a problem hiding this comment.
Instead of checking iPad or size class, maybe we can present the info either in sheet if available, and fallback to popover if sheet is not available?
kean
force-pushed
the
task/remove-bottomvc-1
branch
from
3c85985 to
572ebbc
Compare
| tableView.layoutIfNeeded() | ||
| preferredContentSize = tableView.contentSize | ||
|
|
||
| if UIDevice.isPad() { |
There was a problem hiding this comment.
What do you think changing this condition to something like navigationController?.modalPresentationStyle == .popover?
There was a problem hiding this comment.
Oh, forgot to remove it. It's safe to not have any condition here. preferredContentSize will only be used by the presentation controllers that need it, like the popover. Updated.
|
|
||
| if UIDevice.isPad() { | ||
| preferredContentSize = CGSize(width: 320, height: tableView.contentSize.height) | ||
| } |
There was a problem hiding this comment.
I wonder if you can delete this setting preferredContentSize code if setting tableView.scrollable = false.
There was a problem hiding this comment.
Or other ways to make UITableView display its full content.
I'm not sure using UITableView is a good choice here... I presume UIKit would take autolayout into account when displaying a popover. We probably don't need to set preferredContentSize if using a UIStackView instead.
There was a problem hiding this comment.
This was the existing implementation. I'm not planning to rework it in the scope of this PR.
3 participants
This is some background work to remove
BottomSheetViewController, which is a quite large class that also has a couple of warnings. We don't want to maintain that, and the UIKit components do the same but better now.Before → After (iPhone)
The new version is better for accessibility with a visible close button.
Before → After (iPad)
On iPad, you should just use a popover for this scenarios. It a much quicker and lighter interaction. There are many other cases where we could take some quick wins by simply using popovers on iPad.
To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: