Remove custom navigation bar #5121
Conversation
…ach tab view - also hide tab bar in article view controller on push
- header view still needs work
| // MARK: - Subclass Overrides | ||
|
|
||
| public func appEnvironmentDidChange() { | ||
| overrideUserInterfaceStyle = appEnvironment.theme.userInterfaceStyle |
There was a problem hiding this comment.
Small tiny: can we indent this
| super.viewDidDisappear(animated) | ||
|
|
||
| if parent == nil { | ||
| tappedBack() |
There was a problem hiding this comment.
Should we add any debug prints here or anything?
There was a problem hiding this comment.
@l-olson1214 I think it's okay unless there's something particular you want me to write out here. Before in the old code, we added a custom navigation button that called tappedBack upon tap, which logs analytics that they tapped the back button. From what I could find this is a way to get similar functionality with the system nav back button.
| @@ -0,0 +1,333 @@ | |||
| // Common methods used for navigation bar display in the app | |||
| import UIKit | |||
|
|
|||
There was a problem hiding this comment.
Optional: I think WMFNavigationBarConfiguring deserves to be in it's own file, rather than in a Utils file, same as WMFNavigationBarHiding.
| } | ||
|
|
||
| // TODO: remove after expiry date (1 March 2025) | ||
| func presentYearInReviewAnnouncement() { | ||
|
|
||
| // Do not show announcement when article is previewing. Fixes crash on preview long press. |
| @@ -104,7 +124,7 @@ extension DiffHeaderTitleView: Themeable { | |||
| func apply(theme: Theme) { | |||
|
|
|||
| backgroundColor = theme.colors.paperBackground | |||
| contentView.backgroundColor = theme.colors.paperBackground | |||
| backgroundColor = theme.colors.paperBackground | |||
There was a problem hiding this comment.
Duplicate of line above
Wikipedia/Code/DiffHeaderView.swift
Outdated
| } | ||
|
|
||
| func configure(with vm: DiffHeaderViewModel, titleViewTapDelegate: DiffHeaderTitleViewTapDelegate? = nil, theme: Theme) { | ||
| contentView.configure(with: vm, titleViewTapDelegate: titleViewTapDelegate, theme: theme) |
There was a problem hiding this comment.
Would this (titleViewTapDelegate) be a good candidate for just passing the function through? I know we talked about that the other day in the badge delegate PR
There was a problem hiding this comment.
@l-olson1214 Reworked these to use closures, let me know if you'd like more tweaks.
| @@ -241,8 +233,42 @@ private extension DiffListViewController { | |||
| } | |||
|
|
|||
| extension DiffListViewController: UICollectionViewDataSource { | |||
|
|
|||
| func numberOfSections(in collectionView: UICollectionView) -> Int { | |||
| return 3 | |||
There was a problem hiding this comment.
Why isn't this just a regular variable / computed variable? Why is it a func?
There was a problem hiding this comment.
This one is just conforming to the UIKit UICollectionDataSource protocol, basically it's how we tell the collection view how many sections to display in the list.
It defaults to 1, which is why we didn't have to implement it at all before. With this change, we're taking some header views that were embedded in the custom navigation bar and sticking them in the list as section headers. So now the number of sections went from 1 to 3.
| var delegatesSelection: Bool = false | ||
| var doesShowArticlePreviews = true | ||
|
|
||
| weak var delegate: SearchResultsViewControllerDelegate? |
There was a problem hiding this comment.
Can we just pass the func here
There was a problem hiding this comment.
@l-olson1214 Reworked these to use closures, let me know if you'd like more tweaks.
…chedSelectionDelegate
| class PageHistoryViewController: ColumnarCollectionViewController { | ||
| class PageHistoryViewController: ColumnarCollectionViewController, WMFNavigationBarConfiguring { | ||
|
|
||
| // fileprivate static let headerReuseIdentifier = "TestHeader" |
There was a problem hiding this comment.
Minor: I think this comment can be deleted
There was a problem hiding this comment.
Ah good catch. Fixed!
Phabricator: https://phabricator.wikimedia.org/T363606
Notes
This PR removes the ViewController, NavigationBar, NavigationBarHider, and WMFThemeableNavigationController superclasses and deals with the fallout. There other other smaller cleanup items, but those are the main ones.
I added a WMComponentNavigationController (subclass of UINavigationController) in WMFComponents for us to switch to. All legacy view controllers and newer WMFComponentViewControllers use this for navigating.
I also added a WMFNavigationBarConfiguring and default protocol implementation of
configureNavigationBar(...)that all UIViewControllers can (optionally) call. I also tried to comb through most of the app and use this method in everyviewWillAppear, so that all configuration flows through one method.There are still bugs, code cleanup and design review feedback that I need to do, but I wanted to get this up (at least in draft form) so that we can discuss it and potentially start review.
Test Steps