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: add full screen ad hooks #100

Merged
merged 12 commits into from
Apr 8, 2022
Merged

feat: add full screen ad hooks #100

merged 12 commits into from
Apr 8, 2022

Conversation

wjaykim
Copy link
Collaborator

@wjaykim wjaykim commented Mar 19, 2022

Description

This PR add supports for hook-style ad state management which can be used for full screen ads(AppOpen, Interstitial, Rewarded). This feature is initially implemented in another admob library, @react-native-admob/admob.
I'm happy to join forces on this single package which can be maintained better😁.

Related issues

Release Summary

add full screen ad hooks

Checklist

  • I read the Contributor Guide
    and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in __tests__e2e__
    • jest tests added or updated in __tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

Think react-native-google-mobile-ads is great? Please consider supporting the project with any of the below:

  • 👉 Star this repo on GitHub ⭐️
  • 👉 Follow Invertase on Twitter

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@wjaykim wjaykim changed the title [WIP] feat: add full screen ad hooks feat: add full screen ad hooks Mar 19, 2022
@mikehardy
Copy link
Collaborator

mikehardy commented Mar 19, 2022 •
edited
Loading

@wjaykim welcome welcome 👋 😄 I am really excited personally to (hopefully) have everyone join forces in one library we can all tag-team on so as maintainers we all have just a little to do instead of everyone having to do everything. I'll queue this for review and hopefully the vision of one great admob package and a reasonable work load for everyone can become a reality :)

@codecov
Copy link

codecov bot commented Mar 19, 2022 •
edited
Loading

Codecov Report

Merging #100 (a804a74) into main (7dcf5df) will decrease coverage by 1.77%.
The diff coverage is 1.57%.

❗ Current head a804a74 differs from pull request most recent head b0ce5f5. Consider uploading reports for the commit b0ce5f5 to get more accurate results

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
- Coverage   21.83%   20.06%   -1.76%     
==========================================
  Files          25       29       +4     
  Lines         669      733      +64     
  Branches      171      189      +18     
==========================================
+ Hits          146      147       +1     
- Misses        523      586      +63

wjaykim added 3 commits March 20, 2022 16:39
@wjaykim
fix: set empty object as default requestOptions
564e17a 
useDeepCompareEffect doesn't accept primitive value only as its dependency array.

To avoid that, pass empty object as requestOptions instead of undefined value.
@wjaykim
docs: add documentation for using hooks
3c470a4
@wjaykim
test(example): add hook test
85a5850
@wjaykim wjaykim marked this pull request as ready for review March 30, 2022 10:31
dylancom
dylancom requested changes Apr 8, 2022
View reviewed changes
docs/displaying-ads-hook.mdx Outdated Show resolved Hide resolved
docs/displaying-ads-hook.mdx Outdated Show resolved Hide resolved
docs/displaying-ads-hook.mdx Outdated Show resolved Hide resolved
docs/displaying-ads-hook.mdx Outdated Show resolved Hide resolved
docs/displaying-ads-hook.mdx Outdated Show resolved Hide resolved
src/hooks/useAppOpenAd.ts Outdated Show resolved Hide resolved
src/hooks/useInterstitialAd.ts Outdated Show resolved Hide resolved
src/hooks/useRewardedAd.ts Outdated Show resolved Hide resolved
mikehardy
mikehardy approved these changes Apr 8, 2022
View reviewed changes
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Looks great to me, and @dylancom just had language comments which I went ahead and just committed, since it seemed best to do a squash merge anyway since it's one feature all at once effectively

I'll pull locally, check the example and assuming it looks good merge this

@mikehardy mikehardy added workflow: waiting for feedback Waiting on response to questions workflow: pending merge Waiting on CI or question responses to merge, but otherwise ready and removed workflow: waiting for feedback Waiting on response to questions labels Apr 8, 2022
@mikehardy
Copy link
Collaborator

I tested it locally and everything works - I actually extended the test app a bit to test all 3 of the hooks and they're all good.
I would have included that as a new commit on this PR but I think I saw an error that testing exposed, so I'll do it as another PR.

So this is good to go --> @wjaykim 🏆 🙇

@mikehardy mikehardy merged commit 0bd7ce8 into invertase:main Apr 8, 2022
@mikehardy mikehardy removed the workflow: pending merge Waiting on CI or question responses to merge, but otherwise ready label Apr 8, 2022
@mikehardy mikehardy mentioned this pull request Apr 8, 2022
Merged
@wjaykim wjaykim deleted the ad-hooks branch April 9, 2022 02:26
github-actions bot pushed a commit that referenced this pull request Apr 16, 2022
@semantic-release-bot
chore(release): 5.1.0 [skip ci]
1ef7fa3 
## [5.1.0](v5.0.1...v5.1.0) (2022-04-16)

### Features

* **consent:** add method that returns consent choices ([be967bd](be967bd))
* hooks for all full screen ad types ([#100](#100)) ([0bd7ce8](0bd7ce8))
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Star-dev325 added a commit to Star-dev325/react-native-google-mobile-ads that referenced this pull request Jun 7, 2024
@Star-dev325
chore(release): 5.1.0 [skip ci]
e718718 
## [5.1.0](invertase/react-native-google-mobile-ads@v5.0.1...v5.1.0) (2022-04-16)

### Features

* **consent:** add method that returns consent choices ([be967bd](invertase/react-native-google-mobile-ads@be967bd))
* hooks for all full screen ad types ([#100](invertase/react-native-google-mobile-ads#100)) ([0bd7ce8](invertase/react-native-google-mobile-ads@0bd7ce8))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylancom dylancom dylancom approved these changes

@mikehardy mikehardy mikehardy approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wjaykim @CLAassistant @mikehardy @dylancom