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: mark inset*, marginBlock*, marginInline*, paddingBlock* and paddingInline* props as built-in support #219

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

Conversation

ecreeth
Copy link
Contributor

@ecreeth ecreeth commented Oct 14, 2024

This layout props have been implemented in main (default as of 0.76).

Check 46478 for more information.

Should we continue to test these props?

@ecreeth ecreeth requested a review from necolas as a code owner October 14, 2024 19:27
@ecreeth ecreeth changed the title feat: add new layout props feat: mark inset*, marginBlock*, marginInline*, paddingBlock* and paddingInline* props as built-in support Oct 14, 2024
@ecreeth
Copy link
Contributor Author

ecreeth commented Oct 14, 2024

I'm getting this coverage error:

Jest: "global" coverage threshold for branches (89%) not met: 88.79%

Weird... I didn't add any new functionality.

@necolas
Copy link
Contributor

necolas commented Oct 15, 2024

We haven't been using the built in support because all logical styles aren't fully implemented in RN yet, and we had problems in the past with missing desktop support. I'll ask around and see if we can start to drop some of the polyfills

@@ -1011,6 +1008,7 @@ exports[`properties: logical direction inset: inset vs top 1`] = `
{
"style": {
"bottom": 100,
"insetBlockStart": 3,
Copy link
Contributor

@necolas necolas Oct 15, 2024

Choose a reason for hiding this comment

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

This looks like a regression? Depends on whether RN ignore the logical style in favor of the physical style

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how RN works, so this might be expected.

@@ -1031,6 +1028,7 @@ exports[`properties: logical direction inset: insetBlock vs top 1`] = `
{
"style": {
"bottom": 100,
"insetBlockStart": 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

@ecreeth
Copy link
Contributor Author

ecreeth commented Oct 22, 2024

Looks like a miss the direction prop. Now is supported on both platforms.

See https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/StyleSheet/StyleSheetTypes.d.ts#L114

This was referenced Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants