Conversation
# Conflicts: # WMFComponents/Sources/WMFComponents/Extensions/Navigation Bar/WMFNavigationBarConfiguring.swift # Wikipedia/Code/ExploreViewController.swift
There was a problem hiding this comment.
Pull request overview
This PR updates the Explore tab navigation bar behavior to better support a branded logo on the leading side and to adjust that logo as the user scrolls the feed.
Changes:
- Adds a left navigation bar logo
UIBarButtonIteminExploreViewControllerand wires it to scroll-to-top behavior. - Updates
scrollViewDidScrollto callupdateLogoImageOnScroll(scrollView:)to swap between full and compact logo assets while scrolling. - Refactors the
titleView/longTitleButtonlayout setup (Auto Layout constraints) and applies minor whitespace cleanup.
l-olson1214
requested review from
a team and
mazevedofs
and removed request for
a team
| let titleConfig: WMFNavigationBarTitleConfig = WMFNavigationBarTitleConfig(title: CommonStrings.exploreTabTitle, customView: nil, alignment: .leadingCompact) | ||
|
|
||
| let profileButtonConfig = profileButtonConfig(target: self, action: #selector(userDidTapProfile), dataStore: dataStore, yirDataController: yirDataController, leadingBarButtonItem: nil) | ||
| let profileButtonConfig = profileButtonConfig(target: self, action: #selector(userDidTapProfile), dataStore: dataStore, yirDataController: yirDataController, leadingBarButtonItem: nil) |
There was a problem hiding this comment.
titleConfig no longer uses titleView (customView: nil), but WMFAppViewController updates Explore accessibility via vc.titleButton.accessibilityLabel (WMFAppViewController.m:1730-1733). With this change, titleButton no longer corresponds to a visible/tappable nav bar control, so the accessibility label update won’t apply to the new logo button. Recommend either (a) making the left/logo control be what titleButton returns (e.g., a UIButton as a UIBarButtonItem(customView:)), or (b) updating the accessibility update path to target the new left bar button item.
| logoBarButtonItem.accessibilityLabel = titleButton.accessibilityLabel | ||
| logoBarButtonItem.accessibilityHint = titleButton.accessibilityHint | ||
| logoBarButtonItem.accessibilityIdentifier = titleButton.accessibilityIdentifier |
There was a problem hiding this comment.
logoBarButtonItem.accessibilityLabel/hint/identifier are copied from titleButton, but at the time configureNavigationBar() runs those values are likely still unset (they’re assigned later in WMFAppViewController’s navigationController:willShowViewController:). This can leave the new logo button without the intended accessibility label. Prefer setting the label directly here, or ensure the label is applied/updated when WMFAppViewController sets it (e.g., by using a customView button and letting titleButton reference it).
| logoBarButtonItem.accessibilityLabel = titleButton.accessibilityLabel | |
| logoBarButtonItem.accessibilityHint = titleButton.accessibilityHint | |
| logoBarButtonItem.accessibilityIdentifier = titleButton.accessibilityIdentifier | |
| logoBarButtonItem.accessibilityLabel = CommonStrings.exploreTabTitle | |
| logoBarButtonItem.accessibilityHint = WMFLocalizedString("explore-logo-button-accessibility-hint", value: "Double tap to scroll to the top of the Explore feed.", comment: "Accessibility hint for the Wikipedia logo button in the Explore tab that scrolls the feed to the top.") | |
| logoBarButtonItem.accessibilityIdentifier = "explore-logo-bar-button-item" |
There was a problem hiding this comment.
I think it might feel a little better if you deleted these two lazy properties (longTitleButton and titleView). It is super weird to me that WMFAppViewController changes the title view's accessibility label from the outside (that looks like really old code to me), so I would just move the accessibility assignment directly to ExploreViewController's configureNavigationBar method:
logoBarButtonItem.accessibilityLabel = WMFLocalizedString("home-title-accessibility-label", "Wikipedia, scroll to top of Explore", "Accessibility heading for the Explore page, indicating that tapping it will scroll to the top of the explore page. \"Explore\" is the same as {{msg-wikimedia|Wikipedia-ios-welcome-explore-title}}.");
Phabricator:
https://phabricator.wikimedia.org/T416194
Notes
Test Steps
Screenshots/Videos