-
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
Integrate RemoteMessaging with NewTabPage #3017
Conversation
Refactor PixelFiringAsync and move to Core
Fix test teardown
self.homeMessageViewModels = homePageMessagesConfiguration.homeMessages.compactMap(self.homeMessageViewModel(for:)) | ||
} | ||
|
||
private func homeMessageViewModel(for message: HomeMessage) -> HomeMessageViewModel? { |
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.
This function is copied from HomeMessageViewSectionRenderer
.
@@ -19,13 +19,32 @@ | |||
|
|||
import SwiftUI | |||
import DuckUI | |||
import RemoteMessaging |
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.
Required for Preview
Core/PixelFiringAsync.swift
Outdated
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.
Moved from AdAttributionPixelReporter
.
|
||
public protocol PixelFiring { | ||
|
||
static func fire(_ pixel: Pixel.Event, |
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.
Had to make first param unnamed to avoid name ambiguity, but I think API still makes sense, only downside is that when using Pixel
directly there are both variants available in autocomplete. In the end I believe we should be using protocol exclusively.
static let messageMaximumWidth: CGFloat = 380 | ||
static let messageMaximumWidthPad: CGFloat = 455 |
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.
Used values from HomeMessageCollectionViewCell
.
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.
The pixel thing confused me for a moment. Took me a minute to understand why we need the async one, but I get it. If you think this a pattern we should propagate then please create a diary entry for it. Not sure how compatible it is with the PixelKit work?
Anyway, aside from that, LGTM!
@federicocappelli any thoughts on this? Do you think it clashes with PixelKit in any way? |
Hi @dus7, from my understanding you wanted to:
Your implementation seems good BUT I'm a bit worried about the constant diverging of our approaches across platforms, this could have been a good occasion for adopting PixelKit (only for My point is that I would like to see more collaboration and various teams working on shared tools instead of each of us re-implementing its own thing. This code becomes immediately tech debt, and another blocker, as soon someone tries to adopt PixelKit in iOS. |
Thanks @federicocappelli, this makes sense to me. Completely agreed we should not be diverging our approaches and it wasn't my goal at all while writing this. I wasn't aware about the existence of I'll merge this PR and add a task for myself to add swift concurrency to PixelKit and refactor this code to use PixelKit instead. |
Perfectly understandable! Thank you for putting in the effort! |
* main: (24 commits) Add pixels to measure baseline for New Tab Page sections (#3024) backgrounding UI tests (#3021) improve bookmarks and favorites UI tests (#3019) [DuckPlayer] - 4. Remote Config (#3018) Updates BSK Fix issue when cancelling a download (#3030) Tentative fix for a crash and connectivity issues (#3027) autofill UI tests (#3012) scripts for running UI tests (#3000) Integrate RemoteMessaging with NewTabPage (#3017) Update to iOS 15 deployment target (#3001) Fire a pixel when removing the VPN configuration (#3014) Release 7.127.0-0 (#3020) point to BSK branch (#3016) [DuckPlayer] - 3. URL management & FE comms (#3007) Bump BSK version (#3011) New Tab Page layout and base elements (#3008) Remove Privacy Pro from device once expired account is deleted (#3009) Improvements to subscription settings (#2959) Toggle reports limiter (#3005) ...
Task/Issue URL: https://app.asana.com/0/1206226850447395/1207703703130204/f
Tech Design URL:
CC:
Description:
Adds RemoteMessaging capability to new
NewTabPage
layout.PixelFiring
protocol got reorganized and extended, allowing to abstract this functionality and make depending logic testable. So far it was used only inAdAttributionPixelReporter
.Steps to test this PR:
RemoteMessagingClient.endpoint
value tohttps://www.jsonblob.com/api/1257306235471781888
.Definition of Done (Internal Only):
Device Testing:
Internal references:
Software Engineering Expectations
Technical Design Template