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.

Sign up for GitHub

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

Jump to bottom

Update YGNodeStyleGetGap to return YGValue #1753

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update YGNodeStyleGetGap to return YGValue #1753

wants to merge 2 commits into from
+18 −22

Conversation

heoblitz
Copy link

@heoblitz heoblitz commented Nov 26, 2024 •
edited
Loading

Gap can be styled using both points and percentages, but YGNodeStyleGetGap currently returns a float value.

To maintain alignment with the padding and margin functionalities and allow it to be handled in bridging code, this function has been updated to return YGValue.

@vercel Vercel
Copy link

vercel bot commented Nov 26, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
yoga-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2024 11:55am

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 26, 2024
NickGerleman
NickGerleman approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

Oof. Thanks for noticing this.

It will end up being a breaking change, but we just cut a branch, and I think it would be better to do this then to introduce a new overlap for having the right behavior.

@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NickGerleman NickGerleman mentioned this pull request Nov 26, 2024
Open
NickGerleman pushed a commit to NickGerleman/react-native that referenced this pull request Nov 26, 2024
@heoblitz @facebook-github-bot
Update YGNodeStyleGetGap to return YGValue
f384b73 
Summary:
Gap can be styled using both `points` and `percentages`, but YGNodeStyleGetGap currently returns a float value.

To maintain alignment with the `padding` and `margin` functionalities and allow it to be handled in bridging code, this function has been updated to return YGValue.

X-link: facebook/yoga#1753

Reviewed By: joevilches

Differential Revision: D66513236

Pulled By: NickGerleman
@heoblitz heoblitz closed this Nov 26, 2024
@heoblitz heoblitz reopened this Nov 27, 2024
@heoblitz
Copy link
Author

@NickGerleman I accidentally closed the PR while rebasing.

I've combined the previous commits into one. Please take a look when you have time. Thank you.

@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NickGerleman
Copy link
Contributor

NickGerleman commented Nov 27, 2024 •
edited
Loading

We seem to be crashing in Java bindings with terminating due to uncaught exception of type facebook::yoga::vanillajni::YogaJniException: std::exception

Seems like we still need to update method descriptor for registerNatives? {"jni_YGNodeStyleGetGapJNI", "(JI)F", (void*)jni_YGNodeStyleGetGapJNI}, (it says it's returning a float right now).

@heoblitz
Copy link
Author

heoblitz commented Nov 28, 2024 •
edited
Loading

Oh, I missed that. I've corrected the method descriptor for registerNatives to align with the expected return type. Thank you for pointing it out.

@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

NickGerleman pushed a commit to NickGerleman/react-native that referenced this pull request Dec 3, 2024
@heoblitz @facebook-github-bot
Update YGNodeStyleGetGap to return YGValue
c71eacb 
Summary:
Gap can be styled using both `points` and `percentages`, but YGNodeStyleGetGap currently returns a float value.

To maintain alignment with the `padding` and `margin` functionalities and allow it to be handled in bridging code, this function has been updated to return YGValue.

X-link: facebook/yoga#1753

Reviewed By: joevilches

Differential Revision: D66513236

Pulled By: NickGerleman
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NickGerleman NickGerleman NickGerleman approved these changes

Assignees
No one assigned
Labels
CLA Signed Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@heoblitz @facebook-github-bot @NickGerleman