Add FXIOS-10164 [Homepage] Navigation for Pocket Section #22597
Conversation
|
@OrlaM @adudenamedruby @thatswinnie in case you want to peek before the teams meeting |
|
This pull request has conflicts when rebasing. Could you fix it @cyndichin? 🙏 |
cyndichin
force-pushed
the
cc/FXIOS-10164-navigation-for-pocket-section
branch
3 times, most recently
from
62f639e to
9d1109b
Compare
| @@ -132,7 +131,6 @@ class BrowserCoordinator: BaseCoordinator, | |||
|
|
|||
| func showHomepage() { | |||
| let homepageController = self.homepageViewController ?? HomepageViewController(windowUUID: windowUUID) | |||
| homepageController.parentCoordinator = self | |||
There was a problem hiding this comment.
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.
| @@ -7,6 +7,7 @@ import Storage | |||
| import WebKit | |||
|
|
|||
| import struct MozillaAppServices.CreditCard | |||
| import enum MozillaAppServices.VisitType | |||
There was a problem hiding this comment.
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 VisitType the same
| /// Navigates from home page to a new link | ||
| func navigateFromHomePanel(to url: URL, visitType: VisitType, isGoogleTopSite: Bool) |
There was a problem hiding this comment.
this is specific to homepage today because I am calling the homePanel method in the browser coordinator. It would be great to have a single navigateToLink method instead, but leaving this as of now until we revisit some logic that we are doing in BVC. Let me know if any issues!
| import Foundation | ||
| import Redux | ||
|
|
||
| /// Actions that are related to navigation from the user perspective |
There was a problem hiding this comment.
user perspective for now, we can update this comment if we see that there are API actions taken that uses this
| case _ where state.navigationDestination != nil: | ||
| guard let destination = state.navigationDestination else { return } | ||
| handleNavigation(to: destination) |
There was a problem hiding this comment.
I prefer if statements, but left this for now to follow suite our other cases.
cyndichin
force-pushed
the
cc/FXIOS-10164-navigation-for-pocket-section
branch
from
7fbd8e9 to
61d3ea1
Compare
|
@DanielDervishi @tusharC95 for visibility |
cyndichin
requested review from
mattreaganmozilla,
OrlaM,
adudenamedruby,
MattLichtenstein and
ih-codes
and removed request for
mattreaganmozilla
Client.app: Coverage: 30.63
Generated by 🚫 Danger Swift against c1eef36 |
cyndichin
force-pushed
the
cc/FXIOS-10164-navigation-for-pocket-section
branch
from
61d3ea1 to
c1eef36
Compare
cyndichin
had a problem deploying
to
github-pages
GitHub Actions
Failure
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
📜 Tickets
Jira ticket
Github issue
💡 Description
Background Context
Discuss this PR during the team meeting and agree that this is the path we want to proceed in terms of navigation that is shared across the application. This PR originally came from a question asked in another PR. Some of the logic in that PR gets removed here.
Summary
NavigationBrowserActionand newNavigationDestinationtype that is used to determine how to handle where to navigateHomepageStateknowing about where to navigateBrowserViewControllerStateis responsible for this because the navigation is not unique to homepagehandleNavigation, the effort seems more worth to test this when we revisit cleaning up BVC.📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)Video
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-16.at.14.10.08.mp4