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: flex gap yoga bindings #34360

Conversation

intergalacticspacehighway
Copy link
Contributor

Summary

This PR adds React native binding for Yoga flex gap PR

Changelog

[General] [Added] - Flex gap yoga bindings

Test Plan

Run rn tester app and go to view example. You'll find a flex gap example. Example location - packages/rn-tester/js/examples/View/ViewExample.js

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 6, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Aug 6, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,611,599 +1,266
android hermes armeabi-v7a 7,026,589 +645
android hermes x86 7,911,167 +1,022
android hermes x86_64 7,884,548 +1,100
android jsc arm64-v8a 9,490,011 +1,306
android jsc armeabi-v7a 8,267,272 +694
android jsc x86 9,427,135 +1,057
android jsc x86_64 10,019,990 +1,151

Base commit: 3f8071d
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3f8071d
Branch: main

@cortinico
Copy link
Contributor

(cc @NickGerleman if you want to pick this up).

This can't be merged as it is (I'm closing it for now).

You need to create a PR against https://github.com/facebook/yoga with all the changes inside the ReactCommon/yoga/* folder.

Once that PR lands, you can re-open this one (or send a new PR) and rebase on top of main.
The file you landed in Yoga should auto-sync between the two repos.

@cortinico cortinico closed this Aug 8, 2022
@jacobp100
Copy link
Contributor

jacobp100 commented Aug 15, 2022

@cortinico we have a PR in Yoga - facebook/yoga#1116

Let us know if there’s anything our end we can do to help you guys merge it

@cortinico
Copy link
Contributor

@NickGerleman is taking over that PR. Once it gets merged, let's reopen this one 👍

@NickGerleman
Copy link
Contributor

Thank you for contributing this!

I split out your PR to Yoga, and have different sections out for review right now. I think it should be fully merged this week.

A consideration for this change is that style properties are currently passed to native components as top-level native component properties. So, adding a new style property adds a property to the same namespace folks are using for their custom components. This introduces the potential for collisions, which worries me a bit, especially for gap. It would mean that if someone previously created a component extending something like SimpleViewManager, and used the term previously for one of their own properties, we would introduce a silent conflict.

I've been digging a bit, into the right approach to take with this. E.g. if we may want to change the current style prop scheme, or whether we think this issue is rare enough to punt on it.

The other concern I have is around Fabric compatibility. I can see this PR edits some of the files I would expect to see for wiring the prop in fabric, but I'm not sure it's complete at the moment. E.g. there is a change to YogaStylableProps.cpp to add the property to YogaStylableProps::getDebugProps(), but YogaStylableProps::setProp() in the same file is not updated. We should be sure to validate that everything works as expected on the new architecture.

@NickGerleman NickGerleman reopened this Oct 3, 2022
@github-actions

This comment was marked as outdated.

@intergalacticspacehighway
Copy link
Contributor Author

intergalacticspacehighway commented Oct 13, 2022

Thanks for merging yoga and JNI changes @NickGerleman ❤️. I created a new PR with those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants