Conversation
dan-peluso
left a comment
dan-peluso
left a comment
There was a problem hiding this comment.
Code looks good, but could you upload some before / after screenshots just so I can see the difference
| * | ||
| * @param safeAreaTop Top safe area inset in pixels | ||
| * @param safeAreaBottom Bottom safe area inset in pixels | ||
| */ |
There was a problem hiding this comment.
nit: "Dynamic Island" is iOS terminology — on Android the equivalents are display cutouts / camera holes / punch holes. Might confuse future Android devs reading this.
| FormPosition.TOP, | ||
| FormPosition.TOP_LEFT, | ||
| FormPosition.TOP_RIGHT -> screenHeight - topOffset - formHeight | ||
| FormPosition.TOP_RIGHT -> screenHeight - safeAreaTop - topOffset - formHeight |
There was a problem hiding this comment.
The offset calculation logic (safe area + user offset, gravity-dependent direction, bottom gap for keyboard overlap) has enough moving parts that it'd be easy to regress. Would be nice to have unit tests covering at least the core cases — could extract the pure math into a testable helper. The PR template test checkbox is also unchecked.
(claude review pointed out this is not covered by tests, if that was incorrect, ignore this comment)
sdk/forms/src/main/java/com/klaviyo/forms/presentation/FloatingFormWindow.kt
Show resolved
Hide resolved
evan-masseau
left a comment
evan-masseau
left a comment
There was a problem hiding this comment.
Overall this LGTM. I'd say lets try to boost the tests before the base PR goes into the feature branch, but this one's fine as-is. I will leave a comment with some analysis from Claude about test coverage.
3 participants
Description
handle the safe zones on the native side- add any necessary safe zone to the user provided offset
Just safezone:
Before:
After:
Safezone + User set offset of 100:
Before:
After:
Due Diligence
Release/Versioning Considerations
PatchContains internal changes or backwards-compatible bug fixes.MinorContains changes to the public API.MajorContains breaking changes.Changelog / Code Overview
Test Plan
Related Issues/Tickets