feat: Add string support for aspectRatio #34629

gabrieldonadel · 2022-09-08T03:16:40Z

Summary

This updates aspectRatio to support string values and ratio formats, i.e., '16 / 9', thus aligning it with the CSS Box Sizing Module Level 4 specification as requested on #34425. This also adds unit tests to the processAspectRatio function ensuring the style processing works as expected.

Changelog

[General] [Added] - Add string support for aspectRatio

Test Plan

This can be tested either through processAspectRatio-tests or by using the following code:

 <View
   style={{
     backgroundColor: '#527FE4', 
     aspectRatio: '16 / 9',
  }} />

Screen.Recording.2022-09-08.at.00.47.42.mov

analysis-bot · 2022-09-08T03:40:33Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,727,589	+22
android	hermes	armeabi-v7a	7,130,459	+6
android	hermes	x86	8,034,135	+7
android	hermes	x86_64	8,007,425	+18
android	jsc	arm64-v8a	9,597,018	+100
android	jsc	armeabi-v7a	8,363,335	+106
android	jsc	x86	9,541,246	+95
android	jsc	x86_64	10,133,651	+104

Base commit: 8cdc9e7
Branch: main

analysis-bot · 2022-09-08T04:38:06Z

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

Base commit: 8cdc9e7
Branch: main

facebook-github-bot · 2022-09-12T07:51:56Z

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

cipolleschi · 2022-09-15T11:15:49Z

Libraries/StyleSheet/processAspectRatio.js

+ }
+
+ const match = new RegExp(
+ /(^\s*[+]?\d+([.]\d+)?\s*$)|(^\s*([+]?\d+([.]\d+)?)\s*\/\s*([+]?\d+([.]\d+)?)\s*$)/,


Quoting @jacdebug:

This regex is not easily maintainable also fails on values like auto 4 / 3, can we follow spec and check simple splitting by space will work?
https://css-tricks.com/almanac/properties/a/aspect-ratio/#aa-values
Also this transforms will add up and have some runtime cost, we should do static transforms at build time using babel

@cipolleschi and @jacdebug react native does not really support auto as a value for aspectRatio, any ideas on how we should handle that? Should we default auto to some specific value?

The main problem I can think of about using splitting by space is that it wouldn't be able to handle cases like '4/3', '4 /3' or '4/ 3'

auto can just do nothing since it's the default. And because there is no concept of a "replaced element" in RN, auto <ratio> should probably not be allowed. You can split on auto (to print warning), split on /, trim white space if needed, and parse the floats

necolas · 2022-09-23T22:13:48Z

Libraries/StyleSheet/processAspectRatio.js

+ return aspectRatio;
+ }
+
+ const matches = aspectRatio.split('/').map(s => s.trim());


It actually looks like we don't need to trim, as Number apparently does that internally

facebook-github-bot · 2022-09-26T08:45:01Z

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

cipolleschi · 2022-09-26T08:50:50Z

Hi @gabrieldonadel, thanks for all your work with these PRs. We recently started to maintain the TypeScript types and it looks like that this PR touches the Flow types in the StyleSheetTypes.js. Could you update also the TypeScript definitions in this file?

gabrieldonadel · 2022-09-26T12:15:42Z

Hi @gabrieldonadel, thanks for all your work with these PRs. We recently started to maintain the TypeScript types and it looks like that this PR touches the Flow types in the StyleSheetTypes.js. Could you update also the TypeScript definitions in this file?

Hi @cipolleschi, thanks for reviewing this, I've just pushed a commit updating TS types

facebook-github-bot · 2022-09-26T17:10:46Z

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

cipolleschi · 2022-09-27T09:15:48Z

Hi @gabrieldonadel. We are almost there, many thanks for your patience.

facebook-github-bot · 2022-09-27T09:46:25Z

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

react-native-bot · 2022-09-27T11:15:05Z

This pull request was successfully merged by @gabrieldonadel in 14c91cd.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: This updates `aspectRatio` to support string values and ratio formats, i.e., `'16 / 9'`, thus aligning it with the [CSS Box Sizing Module Level 4](https://drafts.csswg.org/css-sizing-4/#aspect-ratio) specification as requested on facebook#34425. This also adds unit tests to the `processAspectRatio` function ensuring the style processing works as expected. ## Changelog [General] [Added] - Add string support for aspectRatio Pull Request resolved: facebook#34629 Test Plan: This can be tested either through `processAspectRatio-tests` or by using the following code: ```js <View style={{ backgroundColor: '#527FE4', aspectRatio: '16 / 9', }} /> ``` https://user-images.githubusercontent.com/11707729/189029904-da1dc0a6-85de-46aa-8ec2-3567802c8719.mov Reviewed By: jacdebug Differential Revision: D39423304 Pulled By: cipolleschi fbshipit-source-id: d323de93d6524e411e7ab9943335a8ca323b6e61

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 Sep 8, 2022

react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Sep 8, 2022

necolas mentioned this pull request Sep 8, 2022

[w3c] ☂️ Web Styles (Part 1) umbrella issue #34425

Open

74 tasks

cipolleschi requested changes Sep 15, 2022

View reviewed changes

gabrieldonadel force-pushed the feat/aspect-ratio-string-support branch from 2e322ce to 60895da Compare September 23, 2022 21:01

gabrieldonadel requested review from cipolleschi and necolas and removed request for cipolleschi September 23, 2022 21:01

necolas reviewed Sep 23, 2022

View reviewed changes

necolas approved these changes Sep 23, 2022

View reviewed changes

gabrieldonadel requested a review from cipolleschi September 24, 2022 00:01

cipolleschi approved these changes Sep 26, 2022

View reviewed changes

gabrieldonadel force-pushed the feat/aspect-ratio-string-support branch from 60895da to ccf2db5 Compare September 26, 2022 12:14

gabrieldonadel added 3 commits September 26, 2022 16:25

feat: Add string support for aspectRatio

44375f8

chore: Simplify processAspectRatio to not use a custom RegExp

2f8275b

feat: Update TS

e8177d4

gabrieldonadel force-pushed the feat/aspect-ratio-string-support branch from ccf2db5 to e8177d4 Compare September 26, 2022 19:27

facebook-github-bot closed this in 14c91cd Sep 27, 2022

react-native-bot added the Merged This PR has been merged. label Sep 27, 2022

gabrieldonadel deleted the feat/aspect-ratio-string-support branch September 27, 2022 12:06

marklawlor mentioned this pull request Oct 12, 2022

v3 nativewind/nativewind#260

Closed

marklawlor mentioned this pull request Oct 31, 2022

v3 - nativewind@next nativewind/nativewind#308

Closed

mym0404 mentioned this pull request Jan 17, 2023

[0.71.0] Provides aspectRatio conditionally cause js crash #35858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add string support for aspectRatio #34629

feat: Add string support for aspectRatio #34629

gabrieldonadel commented Sep 8, 2022 •

edited

Loading

analysis-bot commented Sep 8, 2022 •

edited

Loading

analysis-bot commented Sep 8, 2022 •

edited

Loading

facebook-github-bot commented Sep 12, 2022

cipolleschi Sep 15, 2022

gabrieldonadel Sep 15, 2022

gabrieldonadel Sep 15, 2022

necolas Sep 23, 2022 •

edited

Loading

gabrieldonadel Sep 23, 2022

necolas Sep 23, 2022

facebook-github-bot commented Sep 26, 2022

cipolleschi commented Sep 26, 2022

gabrieldonadel commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

cipolleschi commented Sep 27, 2022 •

edited

Loading

facebook-github-bot commented Sep 27, 2022

react-native-bot commented Sep 27, 2022

feat: Add string support for aspectRatio #34629

feat: Add string support for aspectRatio #34629

Conversation

gabrieldonadel commented Sep 8, 2022 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Sep 8, 2022 • edited Loading

analysis-bot commented Sep 8, 2022 • edited Loading

facebook-github-bot commented Sep 12, 2022

cipolleschi Sep 15, 2022

Choose a reason for hiding this comment

gabrieldonadel Sep 15, 2022

Choose a reason for hiding this comment

gabrieldonadel Sep 15, 2022

Choose a reason for hiding this comment

necolas Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

gabrieldonadel Sep 23, 2022

Choose a reason for hiding this comment

necolas Sep 23, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Sep 26, 2022

cipolleschi commented Sep 26, 2022

gabrieldonadel commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

cipolleschi commented Sep 27, 2022 • edited Loading

facebook-github-bot commented Sep 27, 2022

react-native-bot commented Sep 27, 2022

gabrieldonadel commented Sep 8, 2022 •

edited

Loading

analysis-bot commented Sep 8, 2022 •

edited

Loading

analysis-bot commented Sep 8, 2022 •

edited

Loading

necolas Sep 23, 2022 •

edited

Loading

cipolleschi commented Sep 27, 2022 •

edited

Loading