-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: replace OnLayoutEvent with SvgOnLayoutEvent #2738
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: replace OnLayoutEvent with SvgOnLayoutEvent #2738
Conversation
4e34ac9
to
c7b0311
Compare
9541ccb
to
89543b6
Compare
We'd love to see it merged as |
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.
The rest of PR looks ok!
x: Int32; | ||
y: Int32; | ||
width: Int32; | ||
height: Int32; |
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.
Are you sure it should be Int instead of Float?
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.
During implementation I was looking at official RN code: https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/OnLayoutEvent.kt
And there they have ints
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.
Also when we look at the creation of SVGOnLayoutEvent
inside VirtualView
we pass to this event Ints:
https://github.com/software-mansion/react-native-svg/pull/2738/files#diff-b99e5fc93019444c57df66f6135a6ff9c28d881abe126fc0adb12773b24a7193R608
1996b31
to
62b9d78
Compare
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.
LGTM
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.
LGTM! Please make sure that it works on both archs and platforms and I think we are good with merging it.
Summary
This PR creates
SvgOnLayoutEvent
and replaces usage ofOnLayoutEvent
for Android because React Native Android team will remove this event from their codebase in near future.Test Plan
Verified that the onLayout event emitted by Svg produces the same payload before and after this change.
Steps:
Result:
Event payload structure and values remained identical
Compatibility
Checklist