-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add FXIOS-10164 [Homepage] Navigation for Pocket Section #22597
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import Storage | |
import WebKit | ||
|
||
import struct MozillaAppServices.CreditCard | ||
import enum MozillaAppServices.VisitType | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a fan of declaring these types from another service, I prefer to have a wrapper object. However, to minimize changing will keep the method for |
||
|
||
protocol BrowserNavigationHandler: AnyObject, QRCodeNavigationHandler { | ||
/// Asks to show a settings page, can be a general settings page or a child page | ||
|
@@ -99,6 +100,9 @@ protocol BrowserNavigationHandler: AnyObject, QRCodeNavigationHandler { | |
|
||
/// Shows the toolbar's search engine selection bottom sheet (iPhone) or popup (iPad) | ||
func showSearchEngineSelection(forSourceView sourceView: UIView) | ||
|
||
/// Navigates from home page to a new link | ||
func navigateFromHomePanel(to url: URL, visitType: VisitType, isGoogleTopSite: Bool) | ||
Comment on lines
+104
to
+105
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is specific to homepage today because I am calling the |
||
} | ||
|
||
extension BrowserNavigationHandler { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// This Source Code Form is subject to the terms of the Mozilla Public | ||
// License, v. 2.0. If a copy of the MPL was not distributed with this | ||
// file, You can obtain one at http://mozilla.org/MPL/2.0/ | ||
|
||
import Common | ||
import Foundation | ||
import Redux | ||
|
||
/// Actions that are related to navigation from the user perspective | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. user perspective for now, we can update this comment if we see that there are API actions taken that uses this |
||
class NavigationBrowserAction: Action { | ||
let url: URL? | ||
init(url: URL? = nil, | ||
windowUUID: WindowUUID, | ||
actionType: ActionType) { | ||
self.url = url | ||
super.init(windowUUID: windowUUID, | ||
actionType: actionType) | ||
} | ||
} | ||
|
||
enum NavigationBrowserActionType: ActionType { | ||
case tapOnCustomizeHomepage | ||
case tapOnCell | ||
case tapOnLink | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
// This Source Code Form is subject to the terms of the Mozilla Public | ||
// License, v. 2.0. If a copy of the MPL was not distributed with this | ||
// file, You can obtain one at http://mozilla.org/MPL/2.0/ | ||
|
||
import Foundation | ||
|
||
/// View types that the browser coordinator can navigate to | ||
enum BrowserNavigationDestination: Equatable { | ||
// Native views | ||
case customizeHomepage | ||
|
||
// Webpage views | ||
case link | ||
} | ||
|
||
/// This type exists as a field on the BrowserViewControllerState | ||
struct NavigationDestination: Equatable { | ||
let destination: BrowserNavigationDestination | ||
let url: URL? | ||
|
||
init( | ||
_ destination: BrowserNavigationDestination, | ||
url: URL? = nil | ||
) { | ||
self.destination = destination | ||
self.url = url | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2064,6 +2064,9 @@ class BrowserViewController: UIViewController, | |
handleNavigationActions(for: state) | ||
case _ where state.displayView != nil: | ||
handleDisplayActions(for: state) | ||
case _ where state.navigationDestination != nil: | ||
guard let destination = state.navigationDestination else { return } | ||
handleNavigation(to: destination) | ||
Comment on lines
+2067
to
+2069
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer if statements, but left this for now to follow suite our other cases. |
||
default: break | ||
} | ||
} | ||
|
@@ -2079,6 +2082,20 @@ class BrowserViewController: UIViewController, | |
store.dispatch(action) | ||
} | ||
|
||
private func handleNavigation(to type: NavigationDestination) { | ||
switch type.destination { | ||
case .customizeHomepage: | ||
navigationHandler?.show(settings: .homePage) | ||
case .link: | ||
guard let url = type.url else { | ||
logger.log("url should not be nil when navigating for a link type", level: .warning, category: .coordinator) | ||
return | ||
} | ||
// TODO: FXIOS-10165 - Pass in the proper values based on top sites and other homepage links | ||
navigationHandler?.navigateFromHomePanel(to: url, visitType: .link, isGoogleTopSite: false) | ||
} | ||
} | ||
|
||
private func handleDisplayActions(for state: BrowserViewControllerState) { | ||
guard let displayState = state.displayView else { return } | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for a coordinator for homepage since the navigation is done on browser view level and there are no navigation currently that is specific to homepage only.