Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): Support maximum height for Inline Adaptive banners #663
base: main
Are you sure you want to change the base?
feat(banner): Support maximum height for Inline Adaptive banners #663
Changes from 4 commits
7c825ff
0f15efe
c6d7303
5cdd8a7
c307c19
caf26dc
ab22522
7f6c3a8
c308c6b
1f09ec7
34f3f90
c3b75bb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a quick hack because there are too many tests to fit on a screen already and they're all 100% pressable area, so you can't scroll the scroll view. This gives you some space on the right to scroll it. Happy to remove, but I found it helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that has been bugging me for a long long time 😆 - thanks for cleaning that up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to call the native prop "maxHeight" but that ends up setting the native view max height too, which isn't what you want in the case where the user sets a height lower than the minimum (32px) because you get a minimum height ad back and then the view crops it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you prefer a constant defined for this magic number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing in the example app, this condition never becomes truthy. So do we really need this extra sizeStrings?
It seems maxAdHeight is always set before sizes.
Android same?
At least in the example app I am always getting this order:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I couldn't reproduce this on iOS or Android, but I was missing the same check on Android initially and running this with ~25k (out of ~700k) Android users in production, I was getting 10s of crashes a day.
My plan is to refactor this to pass an object with the size related props across to native in a single prop, while having separate props exposed on the JS side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again here, how would you prefer a constant defined for the magic number?