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: Add Android Paper implementation of inset logical properties #36242

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gabrieldonadel
Copy link
Contributor

@gabrieldonadel gabrieldonadel commented Feb 22, 2023

Summary:

This PR adds Paper support to inset logical properties on Android as requested on #34425. This implementation includes the addition of the following style properties

  • inset, equivalent to top, bottom, right and left.
  • insetBlock, equivalent to top and bottom.
  • insetBlockEnd, equivalent to bottom.
  • insetBlockStart, equivalent to top.
  • insetInline, equivalent to right and left.
  • insetInlineEnd, equivalent to right or left.
  • insetInlineStart, equivalent to right or left.

One thing that still needs to be figured out is how to respect the same precedence as here when using Paper (cc @NickGerleman)

iOS changes are in a separate PR to facilitate code review #36241

Changelog:

[ANDROID] [ADDED] - Add Paper implementation of inset logical properties

Test Plan:

  1. Open the RNTester app and navigate to the View page
  2. Test the new style properties through the Insets section

image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 22, 2023
@analysis-bot
Copy link

analysis-bot commented Feb 22, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,605,045 +432
android hermes armeabi-v7a 7,918,955 +432
android hermes x86 9,090,917 +436
android hermes x86_64 8,946,005 +440
android jsc arm64-v8a 9,174,237 +404
android jsc armeabi-v7a 8,365,906 +413
android jsc x86 9,231,895 +407
android jsc x86_64 9,490,592 +411

Base commit: a6690bf
Branch: main

@NickGerleman
Copy link
Contributor

Left a comment on the iOS PR around the precedence, but yeah, I think that is the main thing left here. Thank you for splitting these as well!

@gabrieldonadel
Copy link
Contributor Author

gabrieldonadel commented Feb 24, 2023

Hey @NickGerleman, I'm glad to share that I managed to fix the precedence problem on Android as well. Fix this by using a very similar approach to #36241, adding a mPosition param to the LayoutShadowNode. Please let me know if this approach looks good to you.

facebook-github-bot pushed a commit that referenced this pull request Feb 25, 2023
Summary:
This PR adds Paper support to `inset` logical properties on iOS as requested on #34425. This implementation includes the addition of the following style properties

- `inset`, equivalent to `top`, `bottom`, `right` and `left`.
- `insetBlock`, equivalent to `top` and `bottom`.
- `insetBlockEnd`, equivalent to `bottom`.
- `insetBlockStart`, equivalent to `top`.
- `insetInline`, equivalent to `right` and `left`.
- `insetInlineEnd`, equivalent to `right` or `left`.
- `insetInlineStart`, equivalent to `right` or `left`.

Android changes are in a separate PR to facilitate code review #36242

## Changelog

[IOS] [ADDED] - Add Paper implementation of inset logical properties

Pull Request resolved: #36241

Test Plan:
1. Open the RNTester app and navigate to the `View` page
2. Test the new style properties through the `Insets` section

![image](https://user-images.githubusercontent.com/11707729/220512607-a1d89dbe-64db-4140-9fdb-f9d7897fe3bd.png)

Reviewed By: lunaleaps

Differential Revision: D43525110

Pulled By: NickGerleman

fbshipit-source-id: b70b0ef183dcf192b2c3547422bbe161b7bdba50
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.

It looks like there is already a pattern for this in ReactShadowNodeImpl, e.g:

  private final float[] mPadding = new float[Spacing.ALL + 1];
  private final boolean[] mPaddingIsPercent = new boolean[Spacing.ALL + 1];

Could we put these in the same place, with the same implementation?

We should also change up Spacing.ALL. E.g. rename that to Spacing.ALL_EDGES, then make Spacing.ALL include everything (and use it where we support the extra values).

}
}

private void setYogaPosition(int spacingType, Dynamic position) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm looking closer at the impl of this, and spacingType is translated directly to a Yoga Edge.

We already added some more different values to Spacing, but since these can only accept Yoga Edge parameters, is it possible to use a more constrained type? E.g. changing sinks which expect a valid Yoga enum to instead use YogaEdge?

@@ -74,6 +74,7 @@ void setFromDynamic(Dynamic dynamic) {
}

private final MutableYogaValue mTempYogaValue;
private final Dynamic[] mPosition = new Dynamic[Spacing.INLINE_START + 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Dynamic is probably heavier than we need here. Could keep these as MutableYogaValue?

@gabrieldonadel
Copy link
Contributor Author

gabrieldonadel commented Mar 14, 2023

Sorry for the delay on this, I'll try to take a look this week*!

@gabrieldonadel gabrieldonadel force-pushed the add-android-paper-inset-logical-properties branch from 2529150 to ce89208 Compare March 29, 2023 15:24
@gabrieldonadel gabrieldonadel force-pushed the add-android-paper-inset-logical-properties branch from ce89208 to 6f35c5b Compare April 9, 2023 23:58
lunaleaps pushed a commit to lunaleaps/react-native that referenced this pull request Apr 24, 2023
…ook#36241)

Summary:
This PR adds Paper support to `inset` logical properties on iOS as requested on facebook#34425. This implementation includes the addition of the following style properties

- `inset`, equivalent to `top`, `bottom`, `right` and `left`.
- `insetBlock`, equivalent to `top` and `bottom`.
- `insetBlockEnd`, equivalent to `bottom`.
- `insetBlockStart`, equivalent to `top`.
- `insetInline`, equivalent to `right` and `left`.
- `insetInlineEnd`, equivalent to `right` or `left`.
- `insetInlineStart`, equivalent to `right` or `left`.

Android changes are in a separate PR to facilitate code review facebook#36242

## Changelog

[IOS] [ADDED] - Add Paper implementation of inset logical properties

Pull Request resolved: facebook#36241

Test Plan:
1. Open the RNTester app and navigate to the `View` page
2. Test the new style properties through the `Insets` section

![image](https://user-images.githubusercontent.com/11707729/220512607-a1d89dbe-64db-4140-9fdb-f9d7897fe3bd.png)

Reviewed By: lunaleaps

Differential Revision: D43525110

Pulled By: NickGerleman

fbshipit-source-id: b70b0ef183dcf192b2c3547422bbe161b7bdba50
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…ook#36241)

Summary:
This PR adds Paper support to `inset` logical properties on iOS as requested on facebook#34425. This implementation includes the addition of the following style properties

- `inset`, equivalent to `top`, `bottom`, `right` and `left`.
- `insetBlock`, equivalent to `top` and `bottom`.
- `insetBlockEnd`, equivalent to `bottom`.
- `insetBlockStart`, equivalent to `top`.
- `insetInline`, equivalent to `right` and `left`.
- `insetInlineEnd`, equivalent to `right` or `left`.
- `insetInlineStart`, equivalent to `right` or `left`.

Android changes are in a separate PR to facilitate code review facebook#36242

## Changelog

[IOS] [ADDED] - Add Paper implementation of inset logical properties

Pull Request resolved: facebook#36241

Test Plan:
1. Open the RNTester app and navigate to the `View` page
2. Test the new style properties through the `Insets` section

![image](https://user-images.githubusercontent.com/11707729/220512607-a1d89dbe-64db-4140-9fdb-f9d7897fe3bd.png)

Reviewed By: lunaleaps

Differential Revision: D43525110

Pulled By: NickGerleman

fbshipit-source-id: b70b0ef183dcf192b2c3547422bbe161b7bdba50
@necolas
Copy link
Contributor

necolas commented Jun 1, 2023

What are the next steps to get this PR merged?

@NickGerleman
Copy link
Contributor

For 0.72 we took the step of adding Fabric specific typings for this and the others.

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 24, 2023
@necolas
Copy link
Contributor

necolas commented Sep 11, 2023

Do we still need these in Paper? If time is limited, I think we're better off focusing on fleshing out support in Fabric than the legacy renderer.

Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 10, 2024
@gabrieldonadel
Copy link
Contributor Author

Should we still implement these?

@NickGerleman
Copy link
Contributor

Should we still implement these?

As of recent we've been wanting to avoid changes to Paper. Since inset properties are layout only, the Fabric impl we have for specifically that should work across platforms.

The gap we have for logical property new arch support is on borders. Solving that in the short term would like adding support in the Android View View Manager, along with the iOS View ComponentView, to the existing structure resolving edge values.

There's a longer term change, I am trying to allocate time to make, which would eventually move built-in view managers and component views to a "computed style", which would mean view managers and component views only have to deal with the four physical edges.

@github-actions github-actions bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Mar 11, 2024
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.

None yet

5 participants