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 Fabric implementation of inset logical properties #35692

Conversation

gabrieldonadel
Copy link
Collaborator

@gabrieldonadel gabrieldonadel commented Dec 21, 2022

Summary

This PR implements inset logical properties 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.

Changelog

[GENERAL] [ADDED] - Add Fabric 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
Android iOS
1 2

@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 Dec 21, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 9f78517
Branch: main

@analysis-bot
Copy link

analysis-bot commented Dec 21, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,467,354 +632
android hermes armeabi-v7a 7,787,208 +1,235
android hermes x86 8,942,212 +1,549
android hermes x86_64 8,799,859 +1,235
android jsc arm64-v8a 9,652,983 +174
android jsc armeabi-v7a 8,386,725 +717
android jsc x86 9,716,974 +778
android jsc x86_64 10,194,432 +510

Base commit: 9f78517
Branch: main

@pull-bot
Copy link

PR build artifact for d833603 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

}
if (!props.insetBlockEnd.isUndefined()) {
result.position()[YGEdgeBottom] = props.insetBlockEnd;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should give top/bottom precedence over BlockStart/BlockEnd, to match the resolution Yoga will do when given Start/End vs Left/Right. I.e. cardinal direction wins over flow relative direction. The group below this one can be matched for props which lose precedence to ones already set.

I think all the other values should be fine as is. E.g. “insetInlineStart” is the standard conformant name, so it should clobber RN’s “start”. Horizontal/Vertical and All don’t have existing props, so precedence doesn’t matter for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it @NickGerleman, I've just pushed a commit fixing this

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.

Thanks for the contribution. As a heads up, I’m mostly off the grid until January. Your last change should also be ready to merge internally, but I’m wanting to follow up with a screenshot test that we can’t do in OSS right now, that’s on my queue.

@facebook-github-bot
Copy link
Contributor

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

@pull-bot
Copy link

PR build artifact for b4cd397 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Dec 22, 2022
@facebook-github-bot
Copy link
Contributor

@ryancat merged this pull request in 9669c10.

@NickGerleman
Copy link
Contributor

NickGerleman commented Dec 22, 2022

@gabrieldonadel would you be able to make a PR to https://github.com/facebook/react-native-deprecated-modules/tree/main/deprecated-react-native-prop-types with these changes as well? Same for the last PR once that is merged.

There's a non-ideal situation where we deprecated RN's provided prop-types and moved them to a different repo, but we recently un-deprecated it after feedback we removed it too quickly. It is bundled (and enforced) as part of RN again, but we haven't yet brought the code back to the repo.

In the meantime, prop changes end up touching three different repos (react-native, react-native-deprecated-modules, react-native-website). Though the issue will be better once prop-types are removed in a couple releases, or if we can copy the package back to the RN monorepo before then (FYI @hoxyq).

@gabrieldonadel gabrieldonadel deleted the feat/add-inset-logical-properties branch December 22, 2022 13:31
@gabrieldonadel
Copy link
Collaborator Author

Sure thing @NickGerleman, here is the PR -> facebook/react-native-deprecated-modules#12

lunaleaps pushed a commit to lunaleaps/react-native that referenced this pull request May 2, 2023
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…#35692)

Summary:
This PR implements `inset` logical properties 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`.

## Changelog

[GENERAL] [ADDED] - Add Fabric implementation of inset logical properties

Pull Request resolved: facebook#35692

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

<table>
<tr>
    <td>Android</td>
    <td>iOS</td>
</tr>
  <tr>
    <td><img src="https://user-images.githubusercontent.com/11707729/208821212-fbbac6ed-09a4-43f4-ba98-dfd2cbabf044.png"  alt="1" width="360px"   />
    </td>
<td>
<img src="https://user-images.githubusercontent.com/11707729/208816997-ef044140-8824-4b1b-a77b-085f18ea9e0e.png" alt="2" width="360px"  />
</td>
   </tr>
</table>

Reviewed By: NickGerleman

Differential Revision: D42193661

Pulled By: ryancat

fbshipit-source-id: 3db8bcd2c4db0ef4886b9ec49a46424d57362620
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants