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.

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

feat(banner): allow adaptive banners to use their container's width #674

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

youedd
Copy link

@youedd youedd commented Dec 23, 2024

Description

In a project I’m working on, I need to apply horizontal padding to adaptive banners for better integration with my app's UI.

Currently, it's not possible to give adaptive banners a width different from the screen width, as mentioned in the stale issue #287.
If the parent view has a different width, padding, or margin, the banner may not display correctly, and parts of it could be cut off.

This PR adds possibility for adaptive banners to take their container's width, as suggested by @dylancom in #296 (comment))

I believe this should be the default behaviour for adaptive banner. but to avoid introducing a breaking change. I added a new prop adaptiveMode to enable this functionality:

- MAIN_SCREEN (default): Keeps the existing behavior where the banner takes the screen width.
- CONTAINER: Allows the adaptive banners to use their container's width, with the ad reloaded when the width changes.

Related issues

#287

Release Summary

Allow adaptive banners to use their container's width.

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

example.mp4

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

🔥

Copy link

docs-page bot commented Dec 23, 2024

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/react-native-google-mobile-ads~674

Documentation is deployed and generated using docs.page.

@CLAassistant
Copy link

CLAassistant commented Dec 23, 2024

CLA assistant check
All committers have signed the CLA.

@youedd youedd force-pushed the feat/adaptive_banner_with_parent_width branch from 45cd51c to 6684113 Compare December 23, 2024 01:22
@dylancom
Copy link
Collaborator

Hi, thanks for the contribution. This PR is hard to review though with added stylistic changes (spacing etc).
@markwilcox is also busy btw with adding a "width" and "maxHeight" prop to the banner component #663.

@dylancom dylancom requested a review from mikehardy December 23, 2024 10:32
@youedd youedd force-pushed the feat/adaptive_banner_with_parent_width branch from 6684113 to 0869e66 Compare December 23, 2024 15:01
@youedd youedd force-pushed the feat/adaptive_banner_with_parent_width branch from 0869e66 to e0e4866 Compare December 23, 2024 15:05
@youedd
Copy link
Author

youedd commented Dec 23, 2024

The style changes are removed.
Thanks for the feedback @dylancom !

@dylancom
Copy link
Collaborator

Thanks again for your contribution! After discussion with the other maintainers, we’ve decided to proceed with #663, as it is already in development and reflects changes we previously agreed upon. We appreciate your effort and encourage you to keep contributing.

@mikehardy
Copy link
Collaborator

Echoing Dylan - @youedd we really appreciate the contribution, the idea is that #663 should also allow you to solve your use case, but goes farther and allows specification of height as well. So if it solves the width use case you need, but also height then it seems better to go with the more comprehensive solution. If you head over to 663 and it seems like it won't let you set width then our assumption is wrong and we should perhaps tune 663 so it can solve your use case

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.

4 participants