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(border): [android] border-style: none support #47212

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mateoguzmana
Copy link
Contributor

@mateoguzmana mateoguzmana commented Oct 25, 2024

Summary:

As part of #34425, borderStyle value of none should be supported.

In this PR, the BorderStyle enum is extended and the logic is implemented to make the none value work in both BorderDrawable and CSSBackgroundDrawable as currently there is a feature flag enableNewBackgroundAndBorderDrawables controlling which class draws the border. The logic has been implemented in both classes so it works regardless of the feature flag value.

Also, a new style property (borderStyle) has been implemented in the Yoga layer to be able to decide whether to pass layout spacing from the border. Now, if the border style is set to none, no layout space will be consumed.

In this PR the border style value doesn't explicitly expose the newly accepted property none in the types yet as the implementation for iOS is still needed. Notice the FlowFixMe in the examples.

Changelog:

[ANDROID][ADDED] - border-style: none support

Test Plan:

Two new examples are added. One for Text and one for View – both of them are setting some properties for the border which by default makes the components draw a border with a default BorderStyle.SOLID, but since the property borderStyle: 'none' is also passed, that default is overwritten hence you see no border applied.

In the screenshots, the border widths are set to 10px to be able to test that when the border style is set to none, it accounts for the widths not consuming layout space.

Both examples have been tested with the enableNewBackgroundAndBorderDrawables feature flag on and off.

Text Example View Example
Screenshot_1730156439 Screenshot_1730156332

@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 Oct 25, 2024
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.

This causes border not to render, which we want, but doesn't account for border widths still being passed to Yoga and consuming layout space (compared to web where it acts as if there is no border present).

@mateoguzmana
Copy link
Contributor Author

This causes border not to render, which we want, but doesn't account for border widths still being passed to Yoga and consuming layout space (compared to web where it acts as if there is no border present).

Oh, that's quite an interesting point. Will look into that, thanks for the very quick feedback! @NickGerleman

@mateoguzmana mateoguzmana marked this pull request as draft October 25, 2024 22:55
@mateoguzmana mateoguzmana marked this pull request as ready for review October 28, 2024 23:12
@mateoguzmana
Copy link
Contributor Author

@NickGerleman I've updated my approach to account for the widths that were still being passed to Yoga and consuming layout space.

There are still a few things I need to tweak in this PR, but before putting more effort into that, I would appreciate some feedback on whether this approach is heading in the right direction. I've also updated my PR description and examples to demonstrate that it is working correctly on Android.

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.

Adding BorderStyle to Yoga would unfortunately be more involved than what is in this PR. Some of the code here is normally generated, in the Yoga repo (see the copied "generated from enums.py"), and the edits here are currently to private boundary in Yoga we are wanting to move things off of, instead of public API. We also want new code in Yoga to have unit tests (which can be added to OSS repo).

We also don't want to add APIs to Yoga which don't do anything (like, the layout engine doesn't care about whether a border is dotted or solid. It just needs to know the insets the border will take).

I think this would need to be organized as deriving border widths set on Yoga node, from the borderStyle (to know if borders should be ignored). Though we should also be careful not to add too much complexity here, if this change ends up being very involved.

@mateoguzmana mateoguzmana marked this pull request as draft October 31, 2024 09:40
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants