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

Added gap support to rows and columns #959

Closed
wants to merge 3 commits into from

Conversation

stevenkang90
Copy link
Contributor

Summary

YogaLayout 2.0.0 added support for gaps, this PR plugs that into Litho to allow usage in clients

Changelog

Test Plan

Copy link
Contributor

@adityasharat adityasharat left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to Litho. I have left review comments explaining a more appropriate way of adding a new layout prop. Please let me know if you need more context.

protected @Nullable YogaGutter mGap;
protected @Nullable Integer mGapLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

We are in the process of generalising the implementation of LithoNode (i.e. decouple Litho and Yoga) so we don't recommend adding new Yoga props to LithoNode. Instead they should be implemented using the LayoutProps APIs.

New gap, and gapLength APIs can be added to the LayoutProps interface. Then implement the new APIs in CommonProps for the component to write to, and YogaPropsWriter for the YogaNode to be written to.

The API can then be exposed to Row and Column component builders by adding new setters in (ContainerBuilder)[https://github.com/facebook/litho/blob/master/litho-core/src/main/java/com/facebook/litho/Component.java#L2161]. In the implementation it would call the new API on the common props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much for the feedback, I will give this a go and reach out if I need any support

@stevenkang90
Copy link
Contributor Author

@adityasharat I pushed the changes but I'm having issues building the project on master and on the branch to run the unit tests.

e: file:///Users/stevenkang/Projects/litho/litho-core-kotlin/src/main/kotlin/com/facebook/litho/FlexboxContainer.kt:133:13 Type mismatch: inferred type is MutableList<Component>? but Style? was expected

is there some documentation around this?

…onProps and YogaLayoutProps

   * updated LayoutProp, CommonProps, DefaultProps and test with gap logic
   * updated YogaLayoutProps with yoga node call
   * removed setGap logic in LithoNode
@adityasharat
Copy link
Contributor

That is odd. It fails to compile with the changes in this PR or it fails to compile even on master?

In this context there are only a couple of places where mutable list of components are used:

  * updated the api for Column and Row to support pixels and dips
@stevenkang90
Copy link
Contributor Author

That is odd. It fails to compile with the changes in this PR or it fails to compile even on master?

In this context there are only a couple of places where mutable list of components are used:

Hi @adityasharat I've had a few issues building the project to be honest.

  1. I needed to change branch from tag rather than master for the project to build
  2. I had to bump the c++ version in CMakesList.txt
  3. buck is a pain to install on M1 macs, I haven't been able to run the unit tests as it relies on java 8 for buck but my android studio gradle plugin has a minimum of Android 11 (any help with this would be appreciated)

I've managed to deploy and test the changes by integrating them into some feature work so I'm confident it works.

@adityasharat
Copy link
Contributor

I think the fastest way to unblock you would be to import the PR into our source! I'm on it.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@adityasharat merged this pull request in 7644be3.

@stevenkang90 stevenkang90 deleted the gap_support branch October 30, 2023 14:59
@stevenkang90
Copy link
Contributor Author

Hi @adityasharat were these changes ever released, We deployed an internal artifact to get gap support, recently we updated to 0.49.0 and can't see any gap support in that release still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants