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: Linear gradient border styles with BackgroundStyleApplicator #46084

Conversation

intergalacticspacehighway
Copy link
Contributor

@intergalacticspacehighway intergalacticspacehighway commented Aug 18, 2024

Summary:

Fix linear gradient borders with BackgroundStyleApplicator.

After fix

Screenshot 2024-08-18 at 3 44 56 PM

Changelog:

[ANDROID] [FIXED] - Linear gradient border styles

Test Plan:

Test border examples in LinearGradientExample.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 18, 2024
@intergalacticspacehighway intergalacticspacehighway changed the title fix: Linear gradient border styles with background style applicator fix: Linear gradient border styles with BackgroundStyleApplicator Aug 18, 2024
@@ -49,6 +50,13 @@ public object BackgroundStyleApplicator {
ensureCSSBackground(view).color = color ?: Color.TRANSPARENT
}

@JvmStatic
public fun setBackgroundImage(view: View, gradients: Array<Gradient>?): Unit {
if (!gradients.isNullOrEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If input is null or empty we should clear any existing gradients

@@ -49,6 +50,13 @@ public object BackgroundStyleApplicator {
ensureCSSBackground(view).color = color ?: Color.TRANSPARENT
}

@JvmStatic
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We might eventually support background images based on URI, in which case we’d need to change this public API. Can we design in a way that will be future proof to that? Like maybe having a type for background image layer, that can be gradient type now, and uri later?
  2. Can we accept by list instead of array for consistency?

@@ -107,9 +107,9 @@ public void setBackgroundImage(ReactViewGroup view, @Nullable ReadableArray back
ReadableMap gradientMap = backgroundImage.getMap(i);
gradients[i] = new Gradient(gradientMap);
}
view.setGradients(gradients);
view.setBackgroundImage(gradients);
Copy link
Contributor

Choose a reason for hiding this comment

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

The greater context behind BackgroundStyleApplicator is that it works on any view (e.g. Text instead of just View). Once the flag has been enabled a bit longer, my plan has been to start moving all the setters to BaseViewMansger.

This is why the current view managers use BackgroundStyleApplicator directly when the gate is on, instead of calling through any specific underlying views.

@intergalacticspacehighway
Copy link
Contributor Author

@NickGerleman pushed all the mentioned changes. Thanks!

@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.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Aug 29, 2024
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in 20e3f45.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @intergalacticspacehighway in 20e3f45

When will my fix make it into a release? | How to file a pick request?

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. Merged This PR has been 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.

4 participants