Send today's article count and contentType in banner requests - #3954
Conversation
| alreadyVisitedCount: number; | ||
| engagementBannerLastClosedAt?: string; | ||
| subscriptionBannerLastClosedAt?: string; | ||
| weeklyArticleHistory?: WeeklyArticleHistory; |
|
|
||
| export const getDailyArticleCount = (): DailyArticleCount => { | ||
| // Returns undefined if no daily article count in local storage | ||
| export const getDailyArticleCount = (): DailyArticleCount | undefined => { |
There was a problem hiding this comment.
I changed the return type here because undefined is a more deliberate signal that there is no article count. This is consistent with the weekly article count.
|
Could we move this code out of Wait, what? Deleting
|
|
Size Change: -69.7 kB (-5%) ✅ Total Size: 1.36 MB
ℹ️ View Unchanged
|
I've now merged in main, which means the AC code is no longer in App.tsx. |
| export const isNPageOrHigherPageView = (n: number = 2): boolean => { | ||
| // get daily read article count array from local storage | ||
| const [dailyCount = {} as DailyArticle] = getDailyArticleCount(); | ||
| const [dailyCount = {} as DailyArticle] = getDailyArticleCount() || []; |
There was a problem hiding this comment.
I changed the return type here to DailyArticleHistory | undefined because undefined is a more deliberate signal that there is no article count. This is consistent with the weekly article count.
There was a problem hiding this comment.
Just to check thinking here:
undefined = 0? A thought on this is that undefined is also used to indicate that a reader hasn't given consent? Are we happy saying those are the same things?
There was a problem hiding this comment.
getDailyArticleCount returns undefined if there is no article count in local storage, which would be because they haven't consented. On the Supporter Revenue side it's good to be clear about this.
While doing this PR I discovered that the SignInGate (this file) is also using article count - so I've kept the logic the same as before here by defaulting to [].
| return { | ||
| weeklyArticleHistory: window.guardian.weeklyArticleCount, | ||
| dailyArticleHistory: window.guardian.dailyArticleCount, | ||
| }; |
There was a problem hiding this comment.
Why couple these two and not just use getWeeklyArticleCount and getDailyArticleCount separately as two different pieces of state?
A bit of my confusion is that I would expect, consent isn't given to return undefined, rather than EmptyArticleCounts. Decoupling them might help with confusion.
I can see benefit is we used them together, but we don't seem to anywhere in the code, rather we just destructure this type.
There was a problem hiding this comment.
Good question, I spent a while thinking about this!
The banner does use both counts, and so it's quite convenient to pass them through together (from StickBottomBanner, down a few function calls) here: https://github.com/guardian/dotcom-rendering/pull/3954/files#diff-9fb4db8893a570211b30acefe9224ff36a020986088db2ae69a32dc36723c6e7R235
Also it also felt natural to keep the initialisation of these 2 counts together in this one function because it all depends on awaiting hasOptedOutOfArticleCount.
But - I'm not wedded to this implementation if you think it's preferable to split them?
There was a problem hiding this comment.
Sorry, I think I might have conflated things.
I am confused by this method return type as it stands. If someone has not consented, I would expect to get undefined. Instead I get an object with undefined prop values, which feels like they mean that they are available, just not set. So, in my head the return type of ArticleCounts | undefined might make more sense.
Some tests might make this more understandable.
On your point re: async hasOptedOut -> this is true, dotcom could potentially have a better way of sharing this state. Given that that is a potential performance issue, we could just leave it as 1.
There was a problem hiding this comment.
Ah right I see what you mean, I'll have a go at refactoring to return ArticleCounts | undefined
| export const isNPageOrHigherPageView = (n: number = 2): boolean => { | ||
| // get daily read article count array from local storage | ||
| const [dailyCount = {} as DailyArticle] = getDailyArticleCount(); | ||
| const [dailyCount = {} as DailyArticle] = getDailyArticleCount() || []; |
There was a problem hiding this comment.
Just to check thinking here:
undefined = 0? A thought on this is that undefined is also used to indicate that a reader hasn't given consent? Are we happy saying those are the same things?
jamesgorrie
left a comment
There was a problem hiding this comment.
I think this PR surfaces that how we share async state could use some work. I also think it's outside of the scope of this PR.
There's a question on the return type of getArticleCounts.
Thanks for the review! I think I have at least partially addressed the question about |
SDC PR: guardian/support-dotcom-components#624
This PR makes the client send 2 new fields in the targeting payload to SDC for banners: