-
Notifications
You must be signed in to change notification settings - Fork 415
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 feature flag for improved New Tab Page #2980
Conversation
@@ -0,0 +1,56 @@ | |||
// | |||
// ImprovedHomeViewController.swift |
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.
SwiftLint - presume you renamed the file or something during dev
|
||
import SwiftUI | ||
|
||
final class HomeTabViewController: UIHostingController<HomeTabView>, HomeViewControllerProtocol { |
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.
Wonder if we should just go all in on calling it 'new tab page' ? e.g.
NewTabPageViewController
?
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.
Yeah, probably. I was hesitating and left one similar to what's now, but maybe it's better to keep consistency with internal names.
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.
Hey, looks good and works. Needs some renaming, SwiftLint, and resolve conflicts. Give me a ping for a follow up review.
|
||
import UIKit | ||
|
||
protocol HomeViewControllerProtocol: UIViewController { |
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.
Let's avoid calling protocols "Protocol" - I know we have some in the code base already, but imo they need refactored. From the API Design Guidelines:
-
Protocols that describe what something is should read as nouns (e.g. Collection).
-
Protocols that describe a capability should be named using the suffixes able, ible, or ing (e.g. Equatable, ProgressReporting).
I think the protocol here is simply NewTabPage
but open to suggestion.
@@ -41,6 +42,7 @@ class RootDebugViewController: UITableViewController { | |||
case openVanillaBrowser = 670 | |||
case resetSendCrashLogs = 671 | |||
case refreshConfig = 672 | |||
case homeTabImprovements = 674 |
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.
I'm not overly keen on this. Would be better to capture the cell as an outlet and check that. However, looks like we're invested in this approach. We should probably refactor this debug view controller at some point. :)
(No action required)
var homeController: HomeViewControllerProtocol? { | ||
homeViewController ?? homeTabViewController | ||
var newTabPageViewController: NewTabPageViewController? | ||
var homeController: NewTabPage? { |
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.
I left homeController
here deliberately to avoid extensive renaming. It'll be straighten out after the old code is cleaned up.
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.
FYI I pushed a swiftlint fix
LGTM!
* main: Update VPN metadata to include entitlement (#3002) Remove noise from wake pixel (#2986) Release 7.126.0-1 (#2998) [DuckPlayer] - 2. Overlay Views (#2989) update translations for autocomplete settings changes (#2995) Fixes goBack() behavior (#2996) Re-add the autocomplete settings toggle pixels (#2991) Update BSK with autofill 12.0.1 (#2983) Final batch of new autofill pixels (#2977) don't attach a new home screen if autoclear is in progress (#2988) Custom DNS for VPN (#2971) Duck Player settings page (#2990) Add feature flag for improved New Tab Page (#2980)
Task/Issue URL: https://app.asana.com/0/1206226850447395/1207630604691910/f
Tech Design URL:
CC:
Description:
Defines an internal flag to keep existing implementation and make new one possible to enable via debug menu. This allows to do incremental development changes into main branch.
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template