Implement tab bar remote message #3665

jotaemepereira · 2024-12-12T21:01:52Z

Task/Issue URL: https://app.asana.com/0/1204006570077678/1208970712110808/f
Tech Design URL:
CC:

Description
Adds a new remote message that is shown in the tab bar.

Steps to test this PR:

Go to the RemoteMessagingClient and change the DEBUG endpoint URL to https://www.jsonblob.com/api/1316017217598578688
Run the app
You should see the new blue button in the tab bar
When hovering it, it should show a popup.
Tapping the button will take you to the survey
Test that when the button is dismissed is removed in all windows
Test that New Tab Page does not show this new message

Definition of Done:

Does this PR satisfy our Definition of Done?

—

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

github-actions · 2024-12-12T21:37:10Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 7d54df2

ayoy

Hey @jotaemepereira! The message works nicely but l've found a bug in HTML NTP and also wanted to suggest some changes in the view to better support localization. Please have a look, thanks!

ayoy · 2024-12-17T21:36:56Z

DuckDuckGo/RemoteMessaging/TabBarRemoteMessageView.swift

+                })
+            )
+            .onAppear(perform: { onAppear() })
+            .frame(width: 147)


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

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

ayoy · 2024-12-17T21:38:48Z

DuckDuckGo/RemoteMessaging/TabBarRemoteMessageView.swift

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


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)

This button style was removed.

ayoy · 2024-12-17T21:40:05Z

DuckDuckGo/RemoteMessaging/TabBarRemoteMessageView.swift

+import SwiftUI
+
+struct TabBarRemoteMessageView: View {
+    @State private var presentPopup: Bool = false


This seems unused

I removed it.

ayoy · 2024-12-17T21:41:04Z

DuckDuckGo/RemoteMessaging/TabBarRemoteMessageView.swift

+    enum Constants {
+        static let height: CGFloat = 92
+        static let width: CGFloat = 360
+    }


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.

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.

DuckDuckGo/TabBar/Model/TabBarRemoteMessage.swift

ayoy · 2024-12-17T21:53:36Z

DuckDuckGo/NewTabPage/ActiveRemoteMessageModel+NewTabPage.swift

-        $remoteMessage.dropFirst().eraseToAnyPublisher()
+        $remoteMessage
+            .dropFirst()
+            .filter { $0?.isForTabBar == false }


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.

Done 👍🏼

ayoy · 2024-12-17T21:56:22Z

DuckDuckGo/RemoteMessaging/TabBarRemoteMessageView.swift

+            .padding(.top, 2.5)
+            .padding(.bottom, 3)
+            .padding(.horizontal, 7.5)


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.

I removed this button style.

ayoy · 2024-12-17T21:59:31Z

DuckDuckGo/TabBar/View/TabBarRemoteMessagePresenting.swift

+            onClose: {
+                self.tabBarRemoteMessageViewModel.onMessageDismissed()
+                self.removeFeedbackButton()
+            },
+            onTap: { surveyURL in
+                DispatchQueue.main.async {
+                    WindowControllersManager.shared.showTab(with: .contentFromURL(surveyURL, source: .appOpenUrl))
+                }
+                self.tabBarRemoteMessageViewModel.onSurveyOpened()
+                self.removeFeedbackButton()
+            },
+            onHover: {
+                self.startTabBarRemotMessageTimer(message: tabBarRemotMessage)
+            },
+            onHoverEnd: {
+                self.dismissTabBarRemoteMessagePopover()
+            },
+            onAppear: {
+                self.tabBarRemoteMessageViewModel.markTabBarRemoteMessageAsShown()


Passing self in closures to the view that's owned by a controller that's owned by self introduces a reference cycle. It shouldn't be a problem right here but to be on the safe side I'd use weak self.

ayoy · 2024-12-17T22:00:03Z

DuckDuckGo/TabBar/View/TabBarRemoteMessagePresenting.swift

+    private func startTabBarRemotMessageTimer(message: TabBarRemoteMessage) {
+        tabBarRemoteMessagePopoverHoverTimer?.invalidate()
+        tabBarRemoteMessagePopoverHoverTimer = Timer.scheduledTimer(withTimeInterval: 1.5, repeats: false) { _ in
+            self.showTabBarRemotePopup(message)


Same here with weak self

jotaemepereira · 2024-12-19T19:10:00Z

DuckDuckGo/RemoteMessaging/RemoteMessagingClient.swift

@@ -47,9 +47,9 @@ 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")!


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

jotaemepereira · 2024-12-19T19:10:42Z

DuckDuckGo/RemoteMessaging/TabBarRemoteMessageView.swift

+                    .padding(.bottom, 9)
+            }
+        }
+        .frame(width: 360)


ℹ️ Now, we only fix the width of the popover; if the text is larger, the popover should increase its height.

github-actions · 2024-12-27T00:00:31Z

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

jotaemepereira added the pending ship review label Dec 12, 2024

jotaemepereira requested a review from ayoy December 17, 2024 14:16

jotaemepereira added 12 commits December 17, 2024 12:25

Implement tab bar remote message

3283e6e

Change production URL

2a880c0

Add JSON to be used as debug

62c8426

Use raw JSON

aa916e3

Make button as tall as the tab bar

3f07a6d

Minor feedback

2cf6e71

More improvements

2b74542

Update JSON link

ecc4860

Fix JSON

3dadbce

Add unit tests

0499ff2

Revert JSON changes

472e37e

Remove URL extension

52a86be

jotaemepereira force-pushed the juan/poc-new-user-feedfack-point-of-action branch from 269b7a5 to 52a86be Compare December 17, 2024 15:25

jotaemepereira added 4 commits December 17, 2024 12:43

Fix deallocation issue

5aca880

Fix filtering on remote message NTP

ba6eadf

Move models to RemoteMessaging folder

5e8c3b9

Create TabBarRemoteMessagePresentable

3fb8ec2

jotaemepereira force-pushed the juan/poc-new-user-feedfack-point-of-action branch from b3b001b to 3fb8ec2 Compare December 17, 2024 18:59

ayoy requested changes Dec 17, 2024

View reviewed changes

jotaemepereira added 6 commits December 17, 2024 19:47

Push JSON

4914438

Add raw JSON for review build

f8252d1

Address button feedback

f3151be

Make popover resize automatically

cde71c3

Implement new button designs

d68d8a4

Use weak self on closures

8b91013

jotaemepereira force-pushed the juan/poc-new-user-feedfack-point-of-action branch from 353b532 to 8b91013 Compare December 19, 2024 15:31

Create two publishers: one for NTP and another for tab bar

d56ee4d

Address minor design feedback around margins

7d54df2

jotaemepereira requested a review from ayoy December 19, 2024 18:45

jotaemepereira commented Dec 19, 2024

View reviewed changes

github-actions bot added the stale label Dec 27, 2024

jotaemepereira removed the stale label Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tab bar remote message #3665

Implement tab bar remote message #3665

jotaemepereira commented Dec 12, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024 •

edited

Loading

ayoy left a comment

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

ayoy Dec 17, 2024

jotaemepereira Dec 19, 2024

jotaemepereira Dec 19, 2024

jotaemepereira Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 27, 2024

Implement tab bar remote message #3665

Are you sure you want to change the base?

Implement tab bar remote message #3665

Conversation

jotaemepereira commented Dec 12, 2024 • edited Loading

Internal references:

github-actions bot commented Dec 12, 2024 • edited Loading

ayoy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotaemepereira Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 27, 2024

jotaemepereira commented Dec 12, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024 •

edited

Loading

jotaemepereira Dec 19, 2024 •

edited

Loading