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

Fix handling 'auto' checks in absolute layout #1689

Closed
wants to merge 9 commits into from

Conversation

coado
Copy link
Contributor

@coado coado commented Aug 21, 2024

Summary:

Regarding issue with incorrect layout when left is set to auto. This PR introduces handling auto whenever inline or flex position is checked to be defined and it fixes above issue.

Tests:

I have run the provided unit tests and everything passes.

Copy link

vercel bot commented Aug 21, 2024

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 Aug 27, 2024 10:55am

@facebook-github-bot facebook-github-bot added CLA Signed Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 21, 2024
@NickGerleman
Copy link
Contributor

Could you please add tests?

@coado
Copy link
Contributor Author

coado commented Aug 22, 2024

I've added three tests. The first one sets on component left: auto, the second one right: auto, and the last one both left: auto and right: auto. Also, I've noticed that cpp and js test generators were wrongly creating functions that set auto on edges, so I've fixed that. The deployment is failing, so I may be missing something. Please let me know what's wrong and I will fix that.

@NickGerleman
Copy link
Contributor

NickGerleman commented Aug 23, 2024

Looks like the JavaScript binding changes do not compile. See error here: https://github.com/facebook/yoga/actions/runs/10510442433/job/29131043181?pr=1689#logs

javascript/src/Node.h Outdated Show resolved Hide resolved
@NickGerleman
Copy link
Contributor

NickGerleman commented Aug 23, 2024

Yay, that fixed the JavaScript build. But tests in JavaScript fail with an exception, indicating something isn't being bound correctly. https://github.com/facebook/yoga/actions/runs/10521212923/job/29151469086?pr=1689

Summary of all failing tests
 FAIL  tests/generated/YGAbsolutePositionTest.test.ts
  ● absolute_layout_width_height_left_auto_right

    TypeError: root_child0.setPositionAuto is not a function

      88 |     const root_child0 = Yoga.Node.create(config);
      89 |     root_child0.setPositionType(PositionType.Absolute);
    > 90 |     root_child0.setPositionAuto(Edge.Left);
         |                 ^
      91 |     root_child0.setPosition(Edge.Right, 10);
      92 |     root_child0.setWidth(10);
      93 |     root_child0.setHeight(10);

      at Object.setPositionAuto (tests/generated/YGAbsolutePositionTest.test.ts:90:17)

@coado
Copy link
Contributor Author

coado commented Aug 23, 2024

Yay, that fixed the JavaScript build. But tests in JavaScript fail with an exception, indicating something isn't being bound correctly. https://github.com/facebook/yoga/actions/runs/10521212923/job/29151469086?pr=1689

Summary of all failing tests
 FAIL  tests/generated/YGAbsolutePositionTest.test.ts
  ● absolute_layout_width_height_left_auto_right

    TypeError: root_child0.setPositionAuto is not a function

      88 |     const root_child0 = Yoga.Node.create(config);
      89 |     root_child0.setPositionType(PositionType.Absolute);
    > 90 |     root_child0.setPositionAuto(Edge.Left);
         |                 ^
      91 |     root_child0.setPosition(Edge.Right, 10);
      92 |     root_child0.setWidth(10);
      93 |     root_child0.setHeight(10);

      at Object.setPositionAuto (tests/generated/YGAbsolutePositionTest.test.ts:90:17)

Thanks! I pushed change that should fix JS tests


final YogaNode root_child0 = createNode(config);
root_child0.setPositionType(YogaPositionType.ABSOLUTE);
root_child0.setPositionAuto(YogaEdge.LEFT, YogaConstants.AUTOf);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem valid

Copy link
Contributor

@NickGerleman NickGerleman Aug 23, 2024

Choose a reason for hiding this comment

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

More generally, I think these tests assume JNI bindings for the Java Node exist to the new API.

Annoyingly, we don't build or run Java tests in GitHub Actions right now, but it is run when we import it into Phabricator. It's usually pretty straightforward to pattern match these JNI bindings, and I can relay any build/test failures if they happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can see that there is no setPositionAuto function for Java too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! That and generating YogaConstants.AUTOf (with the extraneous "f")

@coado
Copy link
Contributor Author

coado commented Aug 23, 2024

@NickGerleman I've pushed the fix for java. I am unsure if it works 😅

@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 Aug 26, 2024
Summary:
Regarding [issue](facebook#45817) with incorrect layout when `left` is set to `auto`. This PR introduces handling `auto` whenever inline or flex position is checked to be defined and it fixes above issue.

## Tests:
 I have run the provided unit tests and everything passes.

X-link: facebook/yoga#1689

Differential Revision: D61737876

Pulled By: NickGerleman
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in 6d6f69b.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Aug 27, 2024
Summary:
Pull Request resolved: #46216

Regarding [issue](#45817) with incorrect layout when `left` is set to `auto`. This PR introduces handling `auto` whenever inline or flex position is checked to be defined and it fixes above issue.

Changelog:
[General][Fixed] - Fix handling 'auto' checks in absolute layout

## Tests:
 I have run the provided unit tests and everything passes.

X-link: facebook/yoga#1689

Reviewed By: cipolleschi

Differential Revision: D61737876

Pulled By: NickGerleman

fbshipit-source-id: 531199a91c5e122b930b49725ea567cbb1d592ce
facebook-github-bot pushed a commit to facebook/litho that referenced this pull request Aug 27, 2024
Summary:
X-link: facebook/react-native#46216

Regarding [issue](facebook/react-native#45817) with incorrect layout when `left` is set to `auto`. This PR introduces handling `auto` whenever inline or flex position is checked to be defined and it fixes above issue.

Changelog:
[General][Fixed] - Fix handling 'auto' checks in absolute layout

## Tests:
 I have run the provided unit tests and everything passes.

X-link: facebook/yoga#1689

Reviewed By: cipolleschi

Differential Revision: D61737876

Pulled By: NickGerleman

fbshipit-source-id: 531199a91c5e122b930b49725ea567cbb1d592ce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Merged Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants