Skip to content

Add setApplicationMuted and setApplicationVolume methods#228

Merged
rdlabo merged 5 commits intocapacitor-community:masterfrom
ecc521:master
Mar 9, 2023
Merged

Add setApplicationMuted and setApplicationVolume methods#228
rdlabo merged 5 commits intocapacitor-community:masterfrom
ecc521:master

Conversation

@ecc521
Copy link
Contributor

@ecc521 ecc521 commented Feb 16, 2023

Adds methods for setApplicationMuted and setApplicationVolume, implementing #50. The simplest option to me seemed to be simply exposing the AdMob APIs directly here

I can't really figure out how the tests are setup (and they're barely necessary given the simplicity of these changes), so I'd appreciate if someone else could work those in.

Otherwise, all tests that passed with version 4.1.0 are working with this PR. Version 4.1.0 had 2 failed tests, ShowInterstitial. Should not try to call show when no Interstitial was prepared, and ShowRewardVideoAd. Should not try to call show when no Reward was prepared. These still fail with this PR (at least on my device), but none of the code related to either of these tests or methods has been changed (ie, not a regression)

Otherwise, I believe I'd done all the stuff with docs properly. There's a slight opportunity for improving the logging on the web code to log the options object that's passed, but that should be a trivial task to someone better acquainted with this library.

Properly handle invalid parameters (reject, don't crash)
Copy link
Contributor

@distante distante left a comment

Choose a reason for hiding this comment

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

LGTM. The test can be called directly on Android Studio.

They need to be fixed yes, and I think something as important as a monitization plugin really need functional tests 😢.

Alas, due time and health constrains I am not able to look at them.

src/web.ts Outdated
console.log('setApplicationMuted');
}

//TODO: setApplicationVolume takes an options object that should probably be logged as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can log it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented!

Properly log setApplicationVolume and setApplicationMuted calls on web
@ecc521
Copy link
Contributor Author

ecc521 commented Mar 6, 2023

Alright, functions for all platforms are implemented and work fine for me, so I think this is ready to merge. Additionally, the functions properly handle invalid parameters by rejecting rather than crashing the app (whoopsies!)

In the meantime (in case this or a similar PR takes a while to merge), I created an npm package at @ecc521/capacitor-community-admob that implements the changes in this PR I created a package at https://github.com/ecc521/admob/releases/download/v4.1.0/capacitor-community-admob-4.1.0.tgz that can be installed using npm (just install the link)

@distante
Copy link
Contributor

distante commented Mar 7, 2023

@rdlabo if you are OK with the iOS part we can merge this.

@ecc521
Copy link
Contributor Author

ecc521 commented Mar 7, 2023

Just realized one very minor concern - the Android code rejects on an invalid options object, the iOS code uses the default value.

The implementations should probably be brought in line. Any preference? (I lean towards reject)

@ecc521
Copy link
Contributor Author

ecc521 commented Mar 7, 2023

Alright. Implemented the reject behavior so iOS/Android are matching completely.

@rdlabo
Copy link
Member

rdlabo commented Mar 9, 2023

Thanks for great work. looks good. I will merge it!

@rdlabo rdlabo merged commit e3a7b15 into capacitor-community:master Mar 9, 2023
@rdlabo
Copy link
Member

rdlabo commented Mar 9, 2023

I will version each library, so please wait a while for the release.

@ecc521 ecc521 mentioned this pull request Mar 9, 2023
@rdlabo
Copy link
Member

rdlabo commented Mar 10, 2023

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants