news_toolkit icon indicating copy to clipboard operation
news_toolkit copied to clipboard

feat: improve Interstitial Ads implementation

Open matiasleyba opened this issue 2 years ago โ€ข 3 comments

Description

Improve Interstital Ads behavior by adding a logic to display an Interstital Ad every 4 article opens.

The amount of articles before an Ad can be configured by using the variable _numberOfArticleViewsForNewInterstitialAd on ArticleBloc class.

Now the class ArticlePage has a parameter called interstitialAdBehavior whether indicated if the Ad should be shown when the article is opened or when the article is closed.

Demo with ads displayed when opening the article.

https://user-images.githubusercontent.com/18689004/212693387-bffa77a4-c604-4c1f-a4e3-f1f554379dfe.mov

Demo with ads displayed when closing the article.

https://user-images.githubusercontent.com/18689004/212693454-795bd6fb-0829-41d3-9311-8fb9fca4104c.mov

Type of Change

  • [x] โœจ New feature (non-breaking change which adds functionality)
  • [ ] ๐Ÿ› ๏ธ Bug fix (non-breaking change which fixes an issue)
  • [ ] โŒ Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] ๐Ÿงน Code refactor
  • [ ] โœ… Build configuration change
  • [ ] ๐Ÿ“ Documentation
  • [ ] ๐Ÿ—‘๏ธ Chore

matiasleyba avatar Jan 16 '23 13:01 matiasleyba

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jan 16 '23 13:01 google-cla[bot]

Thanks for the contribution! In my opinion, this logic doesn't belong in the AppBloc because it's not app-level state. Instead, I propose adding this logic in the ArticleBloc since we already have an event called ArticleRequested which would allow us to keep track of the article view count. Then, we could use a BlocListener to add an event to the FullScreenAdsBloc instead of always adding the event in initState. Let me know what you think and thanks again!

@felangel At first I thought it was a good idea to keep all on ArticleBloc but then I put the logic on the AppBloc to avoid confusion with the existing article view counter which is used for other features.

Anyway I think it is a good idea move the logic to ArticleBloc since as you said the counter is not app-level state, also removing the logic on initState would be good to keep the widget as Stateless class.

I'll make the change as soon as posible.

Thanks for the review!

matiasleyba avatar Jan 20 '23 19:01 matiasleyba

@felangel Got it, no problem. Should we move the PR to draft?

matiasleyba avatar Jan 24 '23 20:01 matiasleyba