Skip to content
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

Fix controllers not popping when pressing a tab bar button #53

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zats
Copy link
Collaborator

@zats zats commented Mar 1, 2018

This fixes an issue when for some reason (the fact I don't know why bothers me quite a bit) when pressing the tab bar button on the view controller that's already open and drilled down into (for example, Speakers → Particular speaker) normally iOS would pop navigation controller to the root

}

func tabBarController(_ tabBarController: UITabBarController, didSelect viewController: UIViewController) {
if let splitViewController = viewController as? UISplitViewController {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe guard lets and early exits might make this function a bit more readable?

Copy link

@kcastellano kcastellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can chain the guards to remove two of the ifs. For me it seems more readable to have something like this:

guard
    let splitViewController = viewController as? UISplitViewController,
    let navigationController = splitViewController.viewControllers.last as? UINavigationController
    else { return }
    
    
    if navigationController.viewControllers.count > 1 {
        navigationController.popViewController(animated: true)
    } else {
        if let firstController = navigationController.viewControllers.first {
            if let scrollableToTop = firstController as? ScrollableToTop {
                scrollableToTop.scrollAfterTabTap()
            } else {
                firstController.view.findScrollSubview()?.setContentOffset(.zero, animated: true)
            }
        }
        
    }

@zats zats force-pushed the pop-on-tab-bar-selection branch from 4863c93 to 49143f1 Compare March 6, 2018 08:19
Copy link
Collaborator Author

@zats zats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to go and rebase on the latest master

@@ -30,8 +30,6 @@ class ScheduleViewController: ButtonBarPagerTabStripViewController {
buttonBarView.backgroundColor = .white
settings.style.selectedBarBackgroundColor = .white
buttonBarView.selectedBar.backgroundColor = .trySwiftAccentColor()

tabBarController?.delegate = self
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this since it's handled at a higher level now (RootTabBarController delegates this functionality via protocol conformance now)

@@ -77,19 +81,3 @@ private extension ScheduleViewController {
}
}

extension ScheduleViewController: UITabBarControllerDelegate {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the comment above, this functionality is handled in a RootTabBarController now

tableView.estimatedRowHeight = 160
tableView.rowHeight = UITableViewAutomaticDimension
}
}

private extension SessionsTableViewController {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not to brag but phabricator has a feature of hiding whitespace changes in review - it's pretty cool because it minimizes noise like this in review. I remember there was some sort of secret url parameter when reviewing pull requests… but to be honest I don't even know when I would want to see whitespace changes 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zats
Copy link
Collaborator Author

zats commented Mar 6, 2018

So unless anyone has any concerns I'll be merging this in a few days (to give people time to react)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants