Fix #3491: Add New Menu UI #3465

kylehickinson · 2021-03-25T17:08:03Z

Summary of Changes

This pull request fixes #3491

This also changes private mode theming to only affect the main browser toolbars

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()

Test Plan:

Verify all menu options from old design are available in the new design and function correctly
Verify theming works as needed
Verify VPN button toggles correctly, shows loading accordingly while connecting, and shows the correct on/off state when opening the menu
Note: Ref #3491: Move share with button; Fix bookmarks dismiss on navigation #3672 moves the share with… button to the top, which will be inconsistent with screenshots below

Screenshots:

Old (NTP)	New (NTP)

Old (Website)	New (Website)

Old (Website)	New (Website) Expanded

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue is assigned to a milestone (should happen at merge time).

BraveUI/SwiftUI/TableCellButtonStyle.swift

Brandon-T · 2021-03-31T18:28:37Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+Menu.swift

+                        .lineLimit(1)
+                        .foregroundColor(Color(Theme.of(nil).colors.tints.home))
+                }
+                Text(verbatim: url.baseDomain ?? url.absoluteDisplayString)


Maybe url.baseDomain ?? url.host ?? url.absoluteDisplayString? That way you attempt to get base domain else eTLD else display string?

Good call, absoluteDisplayString only strips http/https so host is probably a better fallback

iccub

Found few problems in dark mode,
code looks nice overall

BraveUI/SwiftUI/TableCellButtonStyle.swift

Client/Frontend/Browser/BrowserViewController/BrowserViewController+Menu.swift

iccub · 2021-04-01T11:08:04Z

BraveUI/SwiftUI/TableCellButtonStyle.swift

+    configuration.label
+      .contentShape(Rectangle()) // Needed or taps don't activate on empty space
+      .background(
+        Color(colorScheme == .dark ? .white : .black)


Why not using our theme colors here? do we even have them for 'buttonstyle'

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

soner-yuksel · 2021-04-01T16:56:34Z

Client/Extensions/AppearanceExtensions.swift

@@ -67,9 +67,6 @@ extension Theme {
        InsetButton.appearance(whenContainedInInstancesOf: [SearchSuggestionPromptView.self]).appearanceTextColor = colors.tints.home

        // Overrides all views inside of itself
-        // According to docs, UIWindow override should be enough, but some labels on iOS 13 are still messed up without UIView override as well


The comment is so contradictory I wonder why did they add it to window before :)

Yeah I am not sure, keeping it unfortunately completely breaks SwiftUI re-render after color scheme changes

soner-yuksel · 2021-04-01T16:59:40Z

Client/Frontend/Browser/BrowserViewController.swift

+            activities = shareActivities(for: url, tab: tab, sourceView: view, sourceRect: self.view.convert(self.topToolbar.menuButton.frame, from: self.topToolbar.menuButton.superview), arrowDirection: .up)
+        }
+        let menuController = MenuViewController(content: { menuController in
+            VStack(spacing: 6) {


do you think we should start adding these values in struct or unnecessary for single instance ?

I think it is unnecessary at the moment, as it is a small computed View. If the menu grows to be larger though we can always reconsider

soner-yuksel · 2021-04-01T17:03:52Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+Menu.swift

+    }
+
+    struct MenuTabDetailsView: View {
+        @SwiftUI.Environment(\.colorScheme) var colorScheme: ColorScheme


https://github.com/brave/brave-ios/pull/3465/files#r605576363
Same comment applies here I guess

These are used to force SwiftUI to recompute the body of the View when the users system theme changes (as we don't have an observable "current" theme object anywhere)

soner-yuksel · 2021-04-01T17:08:27Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

        }
+        .buttonStyle(TableCellButtonStyle())
+        .accentColor(Color(Theme.of(nil).colors.tints.home))


why do we use .accentColor here instead of .background(

This accentColor was to set colors of Toggle's to brave's orange, but it doesn't seem to work as Michal tested.

background places a View behind this view, which isn't what we wanted in this case

soner-yuksel · 2021-04-01T17:14:14Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

-    @available(*, unavailable)
-    required init?(coder aDecoder: NSCoder) {
-        fatalError()
+    override var preferredStatusBarStyle: UIStatusBarStyle {


Just curious do we need need to set Status bar style for this controller ?

It is to keep the status bar white when the user navigates to other VC's inside the menu

soner-yuksel · 2021-04-01T17:16:33Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

-    override func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
-        return visibleButtons.count
+    private var isPresentingInnerMenu: Bool {
+        if let _ = presentedViewController as? InnerMenuNavigationController {


can we use is here instead
presentedViewController is InnerMenuNavigationController

soner-yuksel · 2021-04-01T17:21:06Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

+extension MenuViewController: PanModalPresentable {
+    var panScrollable: UIScrollView? {
+        // For SwiftUI:
+        //  - in iOS 13, ScrollView will exist within a host view


Damn thats great info here 👏

soner-yuksel · 2021-04-01T17:24:25Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

+                if let scrollView = view as? UIScrollView {
+                    return scrollView
+                }
+                if !view.subviews.isEmpty, let childScrollView = _scrollViewChild(in: view, depth: depth + 1) {


I have 2 questions here,

First one I guess we are doing this check for iOS 13 only, If thats the case can we mark it like that

Second question is Can there be a situation where we have a scrollview as a direct subview and also scroll view within a hostview? If yes shouldnt we pick the scrollview which exists as direct subview ?

Not 13 only, 14 can also have subviews as subview of the view when we create custom VC's that aren't UITVC's

Yes but we always want the top-level one we find since its the one that controls whether or not panning begins to open/close the pan modal. If its the direct view it will return a few lines above, otherwise we check 1 more level deep. Any other scrolls within are assumed to function separately. I guess one improvement would be to find the first one that scrolls vertically, but Im not sure how we could go about doing that

soner-yuksel · 2021-04-01T17:33:47Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

+            self.panModalSetNeedsLayoutUpdate()
+        }
+        if expandToLongForm {
+            DispatchQueue.main.asyncAfter(deadline: .now() + 0.15) {


why do we delay while xpanding to long form ?

Just makes the animation look nicer, I'll make a comment explaining 🙂

iccub · 2021-04-08T10:59:49Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

+            // Have to increase the content size by the hidden nav bar height so that the size
+            // doesn't change when the user navigates within the menu where the nav bar is visible
+            let navBarHeight = navigationController?.navigationBar.bounds.height ?? 0
+            return CGSize(width: 375, height: min(max(size.height + 16, 240), 580 + navBarHeight))


I see these 580 and 375 magic numbers in few places, would it be worth extracting to a constant?

kylehickinson · 2021-05-05T16:03:33Z

Theme related issues will be solved in #3606

iccub

Needs 1 more rebase, code looks good

kylehickinson force-pushed the new-panel branch 3 times, most recently from f6535f8 to 1e6db6d Compare March 30, 2021 18:13

kylehickinson changed the title ~~New Menu UI~~ Fix #3491: Add New Menu UI Mar 30, 2021

kylehickinson requested review from iccub, Brandon-T and soner-yuksel and removed request for Brandon-T March 30, 2021 18:22

kylehickinson added this to the 1.25 milestone Mar 30, 2021

kylehickinson marked this pull request as ready for review March 30, 2021 18:22

Brandon-T suggested changes Mar 31, 2021

View reviewed changes

iccub suggested changes Apr 1, 2021

View reviewed changes

soner-yuksel reviewed Apr 1, 2021

View reviewed changes

kylehickinson force-pushed the new-panel branch 4 times, most recently from 75ef0a5 to 6e9395d Compare April 6, 2021 17:03

iccub modified the milestones: 1.25, 1.26 Apr 7, 2021

iccub suggested changes Apr 8, 2021

View reviewed changes

kylehickinson force-pushed the new-panel branch from 4f89124 to 9adcf2d Compare April 15, 2021 18:45

kylehickinson force-pushed the new-panel branch from d1edcf1 to d1b79c9 Compare May 5, 2021 15:38

kylehickinson requested review from Brandon-T, iccub and soner-yuksel May 5, 2021 16:01

iccub approved these changes May 6, 2021

View reviewed changes

Brandon-T approved these changes May 6, 2021

View reviewed changes

kylehickinson added 17 commits May 6, 2021 10:39

wip: new menu

5047d64

Only make toolbars purple for private mode

d9a01ad

Theming fixes, iPad fixes

ed00de8

Move settings up; landscape safe area fixes

b36547f

Disable auto-rotate while the new menu is active

b466936

Remove availability checks

0d6d7c1

Cleanup & removal of old menu code

3423344

Move menu building related code into BVC extension

f3231b0

Fix theming issues

3103d88

Fix toggle color resetting

b307bff

Fix VPN VC presentation when tapping VPN button

57e7959

Adjust menu popover layout margins to fix arrow positioning

1f9cbff

Increase size of menu popover on iPad

f3d96b4

Add Playlist item to destinations section

e99f4c6

Increase short form height

72fdcf7

Move magic numbers into variables

ba1b006

Fallback to host first in page activities section

a11cc08

kylehickinson force-pushed the new-panel branch from 3e53733 to a11cc08 Compare May 6, 2021 14:42

kylehickinson merged commit 22a4b0b into development May 6, 2021

kylehickinson deleted the new-panel branch May 6, 2021 15:14

This was referenced May 6, 2021

Provide option to close settings overlay when you are inside nested options #1299

Closed

Refactor Menu Logic #1452

Closed

srirambv mentioned this pull request May 27, 2021

New Menu UI #3491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3491: Add New Menu UI #3465

Fix #3491: Add New Menu UI #3465

kylehickinson commented Mar 25, 2021 •

edited

Loading

Brandon-T Mar 31, 2021

kylehickinson Apr 1, 2021

iccub left a comment

iccub Apr 1, 2021

soner-yuksel Apr 1, 2021

kylehickinson Apr 1, 2021

soner-yuksel Apr 1, 2021

kylehickinson Apr 1, 2021

soner-yuksel Apr 1, 2021

kylehickinson Apr 1, 2021

soner-yuksel Apr 1, 2021

kylehickinson Apr 1, 2021

soner-yuksel Apr 1, 2021

kylehickinson Apr 1, 2021

soner-yuksel Apr 1, 2021

soner-yuksel Apr 1, 2021

soner-yuksel Apr 1, 2021

kylehickinson Apr 1, 2021 •

edited

Loading

soner-yuksel Apr 1, 2021

kylehickinson Apr 1, 2021

iccub Apr 8, 2021 •

edited

Loading

kylehickinson commented May 5, 2021

iccub left a comment

Fix #3491: Add New Menu UI #3465

Fix #3491: Add New Menu UI #3465

Conversation

kylehickinson commented Mar 25, 2021 • edited Loading

Summary of Changes

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iccub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iccub Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

kylehickinson commented May 5, 2021

iccub left a comment

Choose a reason for hiding this comment

kylehickinson commented Mar 25, 2021 •

edited

Loading

kylehickinson Apr 1, 2021 •

edited

Loading

iccub Apr 8, 2021 •

edited

Loading