-
Notifications
You must be signed in to change notification settings - Fork 342
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
fix: add offline support for anonymous targeted user via braze notifications #8603
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 4 Skipped Deployments
|
36575be
to
8ddefc1
Compare
...ledger-live-desktop/src/newArch/features/DynamicContent/components/LogContentCardWrapper.tsx
Outdated
Show resolved
Hide resolved
currentCard?.expiresAt?.getTime() | ||
) { | ||
// support new campaign or resumed campaign with the same id + different expiration date targeting anonymous users | ||
setTimeout(() => { |
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.
Why do we need to use a timeout, please?
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.
so , this timeout is used to wait a little bit before a ui change to mark the content card read (as if it's an api request) , Lucas told me that before the work on this feature it was 3 seconds. It's a time laps to avoid immediate disappear of the dot. Initially i just put 500 ms (like in the video) so it's can disappear in a reasonable time. But i should confirm with Anthony.
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.
simply it will take 3 seconds so that the seen status will be considered for the offline notifications.
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.
Actually it was 2 seconds my bad here is the code used before :
const { notificationsCards, logNotificationImpression, groupNotifications, onClickNotif } =
useNotifications();
const timeoutByUUID = useRef<Record<string, NodeJS.Timeout>>({});
const handleInViewNotif = useCallback(
(visible: boolean, uuid: keyof typeof timeoutByUUID.current) => {
const timeouts = timeoutByUUID.current;
if (notificationsCards.find(n => !n.viewed && n.id === uuid) && visible && !timeouts[uuid]) {
timeouts[uuid] = setTimeout(() => {
logNotificationImpression(uuid);
delete timeouts[uuid];
}, 2000);
}
if (!visible && timeouts[uuid]) {
clearTimeout(timeouts[uuid]);
delete timeouts[uuid];
}
},
[logNotificationImpression, notificationsCards],
);
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.
ok i'll change it ! so that we keep the same look & feel of a real api call , rather than suddenly/fast delete them
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.
✅ Done
9d1a7eb
to
91ee3dd
Compare
98102af
to
52f3b74
Compare
I've tested it but now it seems like with analytics on the notif remains active even if the card has the field viewed sets to true Screen.Recording.2024-12-06.at.10.30.58.mov |
21b1ea0
to
c1a9ed3
Compare
apps/ledger-live-desktop/src/renderer/reducers/dynamicContent.ts
Outdated
Show resolved
Hide resolved
c1a9ed3
to
5c80269
Compare
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.
LGTM
5c80269
to
9a01b8f
Compare
✅ Checklist
npx changeset
was attached.📝 Description
Even when the user opt-out from analytics, it can happen that braze campaign target anonymous user via (anonymous_user_id), in this case notification impression/seen process won't be made by calling the tracking/impression request (as it's an opt-out mode). As a reinforcement layer for this scenario, LLD should support the fact that this notification is read via an store/offline indexation process.
Content card's view state will be conditioned by braze metadata and their existence or not in the
anonymousUserNotifications
store.Test proof 📹
notif_OK_p1.mov
❓ Context
https://ledgerhq.atlassian.net/browse/LIVE-15007
🧐 Checklist for the PR Reviewers