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.

Sign up for GitHub

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

Jump to bottom

feat: ad repeat logic #3894

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: ad repeat logic #3894

wants to merge 8 commits into from
+109 −46

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented Nov 29, 2024 •
edited
Loading

Changes

TODO

  • expose options from settings (replace adSpot)
  • add experiment
  • fix tests
  • placeholder support when ad not available (still fetching)
  • marketing cta and acquisition form support
  • support refreshing ads with new system
  • new repeat logic
  • optimize ad insertion

Logic example, params are [adStart, adRepeat].

adStart defaults to old adSpot param, adRepeat will default to pageSize + 1 for backward compatibility. P is post, A is ad in examples.

[2, 9], page size 8 (backward compatibility example, one ad per page)

P P A P

P P P P

P P P A

P P P P

...

[2, 3], page size 8 (new example, more ads per page, ad shows every 3th post after initial adStart offset)

P P A P

P A P P

A P P A

P P A P

...

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Preview domain

https://ad-repeat-logic.preview.app.daily.dev

@capJavert capJavert self-assigned this Nov 29, 2024
@vercel Vercel
Copy link

vercel bot commented Nov 29, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Dec 2, 2024 4:29pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Dec 2, 2024 4:29pm

@capJavert
Copy link
Contributor Author

Still work in progress but initial logic can be reviewed.

idoshamun
idoshamun reviewed Dec 1, 2024
View reviewed changes
Copy link
Member

@idoshamun idoshamun left a comment

Choose a reason for hiding this comment

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

Overall direction looks good!

capJavert
capJavert commented Dec 2, 2024
View reviewed changes
packages/shared/src/components/Feed.spec.tsx
@@ -423,7 +423,6 @@ describe('Feed logged in', () => {
edges: [defaultFeedPage.edges[0]],
}),
]);
await screen.findByTestId('adItem');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With current logic even though adSpot would be 2 (which means only show ads after 2 posts) we would still show the ad after only once post.

Example with feed page with only one post:
image

With new logic it correctly does not show ad until enough posts are listed:
image

Verified with @idoshamun its ok behaviour.

@capJavert capJavert marked this pull request as ready for review December 2, 2024 16:35
@capJavert capJavert requested a review from a team as a code owner December 2, 2024 16:35
capJavert
capJavert commented Dec 2, 2024
View reviewed changes
packages/shared/src/lib/feed.ts
Comment on lines +214 to +217
export type FeedAdTemplate = {
adStart: number;
adRepeat?: number;
};
Copy link
Contributor Author

@capJavert capJavert Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can make this Record<screen_size, FeedAdTemplate> if we want to customize per tablet/mobile/laptop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah think we need to right, to control existing behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current adSpot, behaviour is static and not per screen size

apps/packages/shared/src/components/Feed.tsx

Line 173 in ca508b4

isSquadFeed || shouldUseListFeedLayout ? 2 : adSpot,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For list and squad we use always 2 so I left that as is since the previous implementation also did not touch it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry not adSpot but adRepeat is dynamic per pagesize now right?
So if we experiment it needs to support screensizes out of box?

sshanzel
sshanzel approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Don't have much to add!

rebelchris
rebelchris requested changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Code wise good, but think we need the screen sizes in the flag to mimic current behaviour.

packages/shared/src/lib/feed.ts
Comment on lines +214 to +217
export type FeedAdTemplate = {
adStart: number;
adRepeat?: number;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah think we need to right, to control existing behaviour?

packages/shared/src/lib/featureManagement.ts
@@ -47,4 +48,8 @@ export const featureOnboardingSources = new Feature(
false,
);

export const featureFeedAdTemplate = new Feature('feed_ad_template', {
adStart: 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this need default position as well here? (to mimic current pagesize)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it depends on pageSize I added a default fallback inside useFeed when adRepeat is undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/dailydotdev/apps/pull/3894/files#diff-e2dd034a87d4027cdddb729a7e7ed2fbd456558adcc0eaeee1364b40dba26417R199

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once adRepeat comes from GB page size does not matter anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebelchris rebelchris rebelchris requested changes

@sshanzel sshanzel sshanzel approved these changes

@omBratteng omBratteng Awaiting requested review from omBratteng omBratteng is a code owner automatically assigned from dailydotdev/web-team

@ilasw ilasw Awaiting requested review from ilasw ilasw is a code owner automatically assigned from dailydotdev/web-team

@nensidosari nensidosari Awaiting requested review from nensidosari nensidosari is a code owner automatically assigned from dailydotdev/web-team

@AmarTrebinjac AmarTrebinjac Awaiting requested review from AmarTrebinjac AmarTrebinjac is a code owner automatically assigned from dailydotdev/web-team

@idoshamun idoshamun Awaiting requested review from idoshamun

Requested changes must be addressed to merge this pull request.

Assignees

@capJavert capJavert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@capJavert @rebelchris @idoshamun @sshanzel