Skip to content
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

Implement tab bar remote message #3665

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
3283e6e
Implement tab bar remote message
jotaemepereira Nov 27, 2024
2a880c0
Change production URL
jotaemepereira Dec 12, 2024
62c8426
Add JSON to be used as debug
jotaemepereira Dec 12, 2024
aa916e3
Use raw JSON
jotaemepereira Dec 12, 2024
3f07a6d
Make button as tall as the tab bar
jotaemepereira Dec 12, 2024
2cf6e71
Minor feedback
jotaemepereira Dec 13, 2024
2b74542
More improvements
jotaemepereira Dec 15, 2024
ecc4860
Update JSON link
jotaemepereira Dec 15, 2024
3dadbce
Fix JSON
jotaemepereira Dec 15, 2024
0499ff2
Add unit tests
jotaemepereira Dec 17, 2024
472e37e
Revert JSON changes
jotaemepereira Dec 17, 2024
52a86be
Remove URL extension
jotaemepereira Dec 17, 2024
5aca880
Fix deallocation issue
jotaemepereira Dec 17, 2024
ba6eadf
Fix filtering on remote message NTP
jotaemepereira Dec 17, 2024
5e8c3b9
Move models to RemoteMessaging folder
jotaemepereira Dec 17, 2024
3fb8ec2
Create TabBarRemoteMessagePresentable
jotaemepereira Dec 17, 2024
4914438
Push JSON
jotaemepereira Dec 17, 2024
f8252d1
Add raw JSON for review build
jotaemepereira Dec 17, 2024
f3151be
Address button feedback
jotaemepereira Dec 18, 2024
cde71c3
Make popover resize automatically
jotaemepereira Dec 19, 2024
d68d8a4
Implement new button designs
jotaemepereira Dec 19, 2024
8b91013
Use weak self on closures
jotaemepereira Dec 19, 2024
d56ee4d
Create two publishers: one for NTP and another for tab bar
jotaemepereira Dec 19, 2024
7d54df2
Address minor design feedback around margins
jotaemepereira Dec 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions DuckDuckGo.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"images" : [
{
"filename" : "Response-DDG-Question-96x96.svg",
"idiom" : "universal"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
5 changes: 4 additions & 1 deletion DuckDuckGo/HomePage/View/HomePageView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,10 @@ extension HomePage.Views {

@ViewBuilder
func remoteMessage() -> some View {
if let remoteMessage = activeRemoteMessageModel.remoteMessage, let modelType = remoteMessage.content, modelType.isSupported {
if let remoteMessage = activeRemoteMessageModel.remoteMessage,
!remoteMessage.isForTabBar,
let modelType = remoteMessage.content,
modelType.isSupported {
ZStack {
RemoteMessageView(viewModel: .init(
messageId: remoteMessage.id,
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/MainWindow/MainViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ final class MainViewController: NSViewController {
self.isBurner = tabCollectionViewModel.isBurner
self.featureFlagger = featureFlagger

tabBarViewController = TabBarViewController.create(tabCollectionViewModel: tabCollectionViewModel)
tabBarViewController = TabBarViewController.create(tabCollectionViewModel: tabCollectionViewModel, activeRemoteMessageModel: NSApp.delegateTyped.activeRemoteMessageModel)
bookmarksBarVisibilityManager = BookmarksBarVisibilityManager(selectedTabPublisher: tabCollectionViewModel.$selectedTabViewModel.eraseToAnyPublisher())

let networkProtectionPopoverManager: NetPPopoverManager = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ import RemoteMessaging

extension ActiveRemoteMessageModel: NewTabPageActiveRemoteMessageProviding {
var remoteMessagePublisher: AnyPublisher<RemoteMessageModel?, Never> {
$remoteMessage.dropFirst().eraseToAnyPublisher()
$remoteMessage
.dropFirst()
.filter { $0?.isForTabBar == false }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this check, but here are 2 more things:

  • This also needs to allow nil values, so { $0?.isForTabBar == false || $0 == nil }
  • This is just a publisher. NewTabPageActiveRemoteMessageProviding protocol also declares remoteMessage accessor which it taken directly from ActiveRemoteMessageModel.remoteMessage. That one isn't filtered in any way and the tab bar message still appears on HTML NTP.

What we've talked about on MM, i.e. adding 2 more variables to ActiveRemoteMessageModel called e.g. newTabPageRemoteMessage and tabBarRemoteMessage that would be updated every time remoteMessage is updated, by filtering that one appropriately, could help solve the problem here and move the filtering logic to the model.

Note that if you add these additional variables, you should update NewTabPageActiveRemoteMessageProviding protocol to use newTabPageRemoteMessage and newTabPageRemoteMessagePublisher but this should be straightforward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍🏼

.eraseToAnyPublisher()
}

func isMessageSupported(_ message: RemoteMessageModel) -> Bool {
Expand Down
7 changes: 7 additions & 0 deletions DuckDuckGo/RemoteMessaging/ActiveRemoteMessageModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,10 @@ extension RemoteMessageModelType {
}
}
}

extension RemoteMessageModel {

var isForTabBar: Bool {
return id == TabBarRemoteMessage.tabBarPermanentSurveyRemoteMessageId
}
}
2 changes: 1 addition & 1 deletion DuckDuckGo/RemoteMessaging/RemoteMessagingClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ final class RemoteMessagingClient: RemoteMessagingProcessing {
static let minimumConfigurationRefreshInterval: TimeInterval = 60 * 30
static let endpoint: URL = {
#if DEBUG
URL(string: "https://raw.githubusercontent.com/duckduckgo/remote-messaging-config/main/samples/ios/sample1.json")!
URL(string: "https://www.jsonblob.com/api/1316017217598578688")!
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This shouldn’t be merged; after approval, I will revert this file to the original URLs.

#else
URL(string: "https://staticcdn.duckduckgo.com/remotemessaging/config/v1/macos-config.json")!
#endif
Expand Down
52 changes: 52 additions & 0 deletions DuckDuckGo/RemoteMessaging/TabBarActiveRemoteMessage.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//
// TabBarActiveRemoteMessage.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import Combine
import RemoteMessaging

protocol TabBarRemoteMessageProviding {
var remoteMessagePublisher: AnyPublisher<RemoteMessageModel?, Never> { get }

func markRemoteMessageAsShown() async
func onSurveyOpened() async
func onMessageDismissed() async
}

final class TabBarActiveRemoteMessage: TabBarRemoteMessageProviding {
private let activeRemoteMessageModel: ActiveRemoteMessageModel

var remoteMessagePublisher: AnyPublisher<RemoteMessageModel?, Never> {
activeRemoteMessageModel.$remoteMessage.eraseToAnyPublisher()
}

init(activeRemoteMessageModel: ActiveRemoteMessageModel) {
self.activeRemoteMessageModel = activeRemoteMessageModel
}

func markRemoteMessageAsShown() async {
await activeRemoteMessageModel.markRemoteMessageAsShown()
}

func onSurveyOpened() async {
await activeRemoteMessageModel.dismissRemoteMessage(with: .primaryAction)
}

func onMessageDismissed() async {
await activeRemoteMessageModel.dismissRemoteMessage(with: .close)
}
}
162 changes: 162 additions & 0 deletions DuckDuckGo/RemoteMessaging/TabBarRemoteMessageView.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
//
// TabBarRemoteMessageView.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import SwiftUI

struct TabBarRemoteMessageView: View {
@State private var presentPopup: Bool = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unused

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it.


let model: TabBarRemoteMessage
let onClose: () -> Void
let onTap: (URL) -> Void
let onHover: () -> Void
let onHoverEnd: () -> Void
let onAppear: () -> Void

var body: some View {
HStack {
Button(model.buttonTitle) {
onTap(model.surveyURL)
}
.buttonStyle(DefaultActionButtonStyle(
enabled: true,
onClose: { onClose() },
onHoverStart: {
onHover()
},
onHoverEnd: {
onHoverEnd()
})
)
.onAppear(perform: { onAppear() })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could simplify this a bit by passing closures directly as arguments:

Suggested change
.buttonStyle(DefaultActionButtonStyle(
enabled: true,
onClose: { onClose() },
onHoverStart: {
onHover()
},
onHoverEnd: {
onHoverEnd()
})
)
.onAppear(perform: { onAppear() })
.buttonStyle(DefaultActionButtonStyle(enabled: true, onClose: onClose, onHoverStart: onHover, onHoverEnd: onHoverEnd))
.onAppear(perform: onAppear)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This button style was removed.

.frame(width: 147)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would that behave with translations? Are we fine with truncating the text?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed. Now, both the button and the popover will resize accordingly.

}
}
}

struct TabBarRemoteMessagePopoverContent: View {
enum Constants {
static let height: CGFloat = 92
static let width: CGFloat = 360
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, would be great to test it with long message bodies. Perhaps we'd want a variable height. The icon should probably stay aligned to the top, like in other remote messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed, now we do not have fixed a fixed height for the popover, the popover will increase in height in case it spans more lines.


let model: TabBarRemoteMessage

var body: some View {
HStack(alignment: .center, spacing: 0) {
Image(.daxResponse)
.resizable()
.scaledToFit()
.frame(width: 72, height: 72)
.padding(.leading, 8)
.padding(.trailing, 16)

VStack(alignment: .leading, spacing: 0) {
Text(model.popupTitle)
.font(.system(size: 13, weight: .bold))
.padding(.bottom, 8)

Text(model.popupSubtitle)
.font(.system(size: 13, weight: .medium))
}
.padding(.trailing, 12)
.padding([.bottom, .top], 10)
}
.frame(width: Constants.width, height: Constants.height)
}
}

private struct DefaultActionButtonStyle: ButtonStyle {

public let enabled: Bool
public let onClose: () -> Void
public let onHoverStart: () -> Void
public let onHoverEnd: () -> Void

public init(
enabled: Bool,
onClose: @escaping () -> Void,
onHoverStart: @escaping () -> Void = {},
onHoverEnd: @escaping () -> Void = {}
) {
self.enabled = enabled
self.onClose = onClose
self.onHoverStart = onHoverStart
self.onHoverEnd = onHoverEnd
}

public func makeBody(configuration: Self.Configuration) -> some View {
ButtonContent(
configuration: configuration,
enabled: enabled,
onClose: onClose,
onHoverStart: onHoverStart,
onHoverEnd: onHoverEnd
)
}

struct ButtonContent: View {
let configuration: Configuration
let enabled: Bool
let onClose: () -> Void
let onHoverStart: () -> Void
let onHoverEnd: () -> Void

@State private var isHovered: Bool = false

var body: some View {
let enabledBackgroundColor = configuration.isPressed
? Color("PrimaryButtonPressed")
: (isHovered
? Color("PrimaryButtonHover")
: Color("PrimaryButtonRest"))

let disabledBackgroundColor = Color.gray.opacity(0.1)
let enabledLabelColor = configuration.isPressed ? Color.white.opacity(0.8) : Color.white
let disabledLabelColor = Color.primary.opacity(0.3)

HStack(spacing: 5) {
configuration.label
.font(.system(size: 13))
.multilineTextAlignment(.center)
.fixedSize(horizontal: false, vertical: true)

Button(action: { onClose() }) {
Image(.close)
}
.frame(width: 16, height: 16)
.buttonStyle(PlainButtonStyle())
}
.frame(minWidth: 44)
.padding(.top, 2.5)
.padding(.bottom, 3)
.padding(.horizontal, 7.5)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm not sure if the design wants fraction paddings (or even odd numbers), so perhaps it's worth checking if 2, 4, and 8 would work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this button style.

.background(enabled ? enabledBackgroundColor : disabledBackgroundColor)
.foregroundColor(enabled ? enabledLabelColor : disabledLabelColor)
.cornerRadius(5)
.onHover { hovering in
isHovered = hovering
if hovering {
onHoverStart()
} else {
onHoverEnd()
}
}
}
}
}
26 changes: 26 additions & 0 deletions DuckDuckGo/TabBar/Model/TabBarRemoteMessage.swift
ayoy marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//
// TabBarRemoteMessage.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

struct TabBarRemoteMessage {
static let tabBarPermanentSurveyRemoteMessageId = "macos_permanent_survey_tab_bar"

let buttonTitle: String
let popupTitle: String
let popupSubtitle: String
let surveyURL: URL
}
Loading
Loading