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

Add Fabric implementation of flow-relative padding and margin #35342

Closed
wants to merge 1 commit into from

Conversation

NickGerleman
Copy link
Contributor

@NickGerleman NickGerleman commented Nov 14, 2022

Summary:
This is a native implementation of the JS shimmed layout-specific properties in #35316.

There is an experiment splitting the prop setting codepath in Fabric, so this change effectively has two implementations depending on whether enablePropIteratorSetter is enabled.

None of these changes make sense to propagate Yoga. inlineEnd, etc are already mapped to YGEdgeStart and YGEdgeEnd, but RN's mapping used a different name. Then blockStart, blockEnd, map directly to existing edges in all cases since we don't support a writing mode.

On web, the last value in the style which computes the given dimension is given precedence. E.g. if "left" comes after "start" it will be chosen. Yoga stylesheets are unordered, and precedence for edges is given based on specificity (left > start > horizontal > all). The JS change would always give precedence to flow-relative properties, and we emulate the same ordering independence for consistency with current rules for resolution.

Changelog:
[General][Added] - Add Fabric implementation of flow-relative padding and margin

Differential Revision: D41267765

@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. p: Facebook Partner: Facebook Partner fb-exported labels Nov 14, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D41267765

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D41267765

NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Nov 14, 2022
…acebook#35342)

Summary:
Pull Request resolved: facebook#35342

This is a native implementation of the JS shimmed layout-specific properties in facebook#35316.

There is an experiment splitting the prop splitting codepath in Fabric, so this change effectively has two implementations depending on whether `enablePropIteratorSetter` is enabled.

None of these changes make sense to propagate Yoga. `inlineEnd`, etc are already mapped to `YGEdgeStart` and `YGEdgeEnd`, but RN's mapping used a different name. Then `blockStart`, `blockEnd`, map directly to existing edges in all cases since we don't support a writing mode.

On web, the last value in the style which computes the given dimension is given precedence. E.g. if "left" comes after "start" it will be chosen. Yoga stylesheets are unordered, and precedence for edges is given based on specificity (start > left > horizontal > all). The JS change would always give precedence to flow-relative properties, and we emulate the same ordering independence for consistency with current rules for resolution.

Changelog:
[General][Added] - Add Fabric implementation of flow-relative padding and margin

Differential Revision: D41267765

fbshipit-source-id: 79d2161a8e541ec6bada9d2800232e59fd05a33c
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Nov 14, 2022
…acebook#35342)

Summary:
Pull Request resolved: facebook#35342

This is a native implementation of the JS shimmed layout-specific properties in facebook#35316.

There is an experiment splitting the prop splitting codepath in Fabric, so this change effectively has two implementations depending on whether `enablePropIteratorSetter` is enabled.

None of these changes make sense to propagate Yoga. `inlineEnd`, etc are already mapped to `YGEdgeStart` and `YGEdgeEnd`, but RN's mapping used a different name. Then `blockStart`, `blockEnd`, map directly to existing edges in all cases since we don't support a writing mode.

On web, the last value in the style which computes the given dimension is given precedence. E.g. if "left" comes after "start" it will be chosen. Yoga stylesheets are unordered, and precedence for edges is given based on specificity (start > left > horizontal > all). The JS change would always give precedence to flow-relative properties, and we emulate the same ordering independence for consistency with current rules for resolution.

Changelog:
[General][Added] - Add Fabric implementation of flow-relative padding and margin

Differential Revision: D41267765

fbshipit-source-id: 5ed6b96802b7ae67c1bddbdd3d058def6ed62b93
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D41267765

@@ -69,9 +69,15 @@ export interface FlexStyle {
| undefined;
left?: number | string | undefined;
margin?: number | string | undefined;
marginBlock?: number | string | undefined;
Copy link
Contributor

@Pranav-yadav Pranav-yadav Nov 15, 2022

Choose a reason for hiding this comment

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

As we know, this type;

number | string | undefined

is a generic type for most of the style attributes.
Should we declare a generic type for this type as well? Ex. BoxDimensionValue or BoxDimensionType etc.

And use the same generic type in below code. This will allow us to DRY the code.

Similarly for ;

number | undefined

What do you think?
PS: It looks like decl file is modified manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Contributions welcome to clean up the TS declarations 🙂.

NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Nov 16, 2022
…ok#35342)

Summary:
Pull Request resolved: facebook#35342

This is a native implementation of the JS shimmed layout-specific properties in facebook#35316.

There is an experiment splitting the prop splitting codepath in Fabric, so this change effectively has two implementations depending on whether `enablePropIteratorSetter` is enabled.

None of these changes make sense to propagate Yoga. `inlineEnd`, etc are already mapped to `YGEdgeStart` and `YGEdgeEnd`, but RN's mapping used a different name. Then `blockStart`, `blockEnd`, map directly to existing edges in all cases since we don't support a writing mode.

On web, the last value in the style which computes the given dimension is given precedence. E.g. if "left" comes after "start" it will be chosen. Yoga stylesheets are unordered, and precedence for edges is given based on specificity (left > start > horizontal > all).

We give precedence to new renamings (e.g. start to inlineStart), but to preserve consistent behavior, we give precedence to specific edges before overwriting them with flow relative ones (e.g. marginTop has precedence over marginBlockStar).

Changelog:
[General][Added] - Add Fabric implementation of flow-relative padding and margin

Differential Revision: D41267765

fbshipit-source-id: 471e3d6c1a9cbb9e544dda1db2ab5b311b9be820
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D41267765

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,101,159 +1,905
android hermes armeabi-v7a 6,470,424 +1,433
android hermes x86 7,517,968 +831
android hermes x86_64 7,376,869 +1,050
android jsc arm64-v8a 8,965,924 +1,935
android jsc armeabi-v7a 7,696,853 +1,378
android jsc x86 9,027,539 +1,001
android jsc x86_64 9,505,834 +1,152

Base commit: 5fda72f
Branch: main

…ok#35342)

Summary:
Pull Request resolved: facebook#35342

This is a native implementation of the JS shimmed layout-specific properties in facebook#35316.

There is an experiment splitting the prop splitting codepath in Fabric, so this change effectively has two implementations depending on whether `enablePropIteratorSetter` is enabled.

None of these changes make sense to propagate Yoga. `inlineEnd`, etc are already mapped to `YGEdgeStart` and `YGEdgeEnd`, but RN's mapping used a different name. Then `blockStart`, `blockEnd`, map directly to existing edges in all cases since we don't support a writing mode.

On web, the last value in the style which computes the given dimension is given precedence. E.g. if "left" comes after "start" it will be chosen. Yoga stylesheets are unordered, and precedence for edges is given based on specificity (left > start > horizontal > all).

We give precedence to new renamings (e.g. start to inlineStart), but to preserve consistent behavior, we give precedence to specific edges before overwriting them with flow relative ones (e.g. marginTop has precedence over marginBlockStar).

Changelog:
[General][Added] - Add Fabric implementation of flow-relative padding and margin

Differential Revision: D41267765

fbshipit-source-id: 2d21c6acb710b04a8577e98bf5cc7d4bb2de1815
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D41267765

@NickGerleman NickGerleman changed the title Draft: Add Fabric implementation of flow-relative padding and margin Add Fabric implementation of flow-relative padding and margin Nov 16, 2022
@analysis-bot
Copy link

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

Base commit: 5fda72f
Branch: main

@pull-bot
Copy link

PR build artifact for d8c9b1d 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.

@pull-bot
Copy link

PR build artifact for d8c9b1d 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.

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

Summary:
Pull Request resolved: facebook#35342

This is a native implementation of the JS shimmed layout-specific properties in facebook#35316.

There is an experiment splitting the prop splitting codepath in Fabric, so this change effectively has two implementations depending on whether `enablePropIteratorSetter` is enabled.

None of these changes make sense to propagate Yoga. `inlineEnd`, etc are already mapped to `YGEdgeStart` and `YGEdgeEnd`, but RN's mapping used a different name. Then `blockStart`, `blockEnd`, map directly to existing edges in all cases since we don't support a writing mode.

On web, the last value in the style which computes the given dimension is given precedence. E.g. if "left" comes after "start" it will be chosen. Yoga stylesheets are unordered, and precedence for edges is given based on specificity (left > start > horizontal > all).

We give precedence to new renamings (e.g. start to inlineStart), but to preserve consistent behavior, we give precedence to specific edges before overwriting them with flow relative ones (e.g. marginTop has precedence over marginBlockStar).

Changelog:
[General][Added] - Add Fabric implementation of flow-relative padding and margin

Reviewed By: javache

Differential Revision: D41267765

fbshipit-source-id: 896e2ed71fe8bf83aef00b0a9d70fd20b2ce47a7
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. fb-exported p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants