Skip to content

feat: Add string support for aspectRatio - #34629

Closed
gabrieldonadel wants to merge 3 commits into
react:mainfrom
gabrieldonadel:feat/aspect-ratio-string-support
Closed

feat: Add string support for aspectRatio#34629
gabrieldonadel wants to merge 3 commits into
react:mainfrom
gabrieldonadel:feat/aspect-ratio-string-support

Conversation

@gabrieldonadel

@gabrieldonadelgabrieldonadel commented Sep 8, 2022

Copy link
Copy Markdown
Collaborator

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:

<Viewstyle={{backgroundColor: '#527FE4',aspectRatio: '16 / 9',}}/>
Screen.Recording.2022-09-08.at.00.47.42.mov

@facebook-github-botfacebook-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
@analysis-bot

analysis-bot commented Sep 8, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,727,589+22
androidhermesarmeabi-v7a7,130,459+6
androidhermesx868,034,135+7
androidhermesx86_648,007,425+18
androidjscarm64-v8a9,597,018+100
androidjscarmeabi-v7a8,363,335+106
androidjscx869,541,246+95
androidjscx86_6410,133,651+104

Base commit: 8cdc9e7
Branch: main

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

analysis-bot commented Sep 8, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: 8cdc9e7
Branch: main

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

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'

@necolasnecolasSep 23, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Updated

@gabrieldonadel
gabrieldonadelforce-pushed the feat/aspect-ratio-string-support branch from 2e322ce to 60895daCompareSeptember 23, 2022 21:01
@gabrieldonadel
gabrieldonadel requested review from cipolleschi and necolas and removed request for cipolleschiSeptember 23, 2022 21:01

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@cipolleschi

Copy link
Copy Markdown
Contributor

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
gabrieldonadelforce-pushed the feat/aspect-ratio-string-support branch from 60895da to ccf2db5CompareSeptember 26, 2022 12:14
@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

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

Copy link
Copy Markdown
Contributor

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

@gabrieldonadel
gabrieldonadelforce-pushed the feat/aspect-ratio-string-support branch from ccf2db5 to e8177d4CompareSeptember 26, 2022 19:27
@cipolleschi

cipolleschi commented Sep 27, 2022

Copy link
Copy Markdown
Contributor

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

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@react-native-bot

Copy link
Copy Markdown
Collaborator

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

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

@react-native-botreact-native-bot added the Merged This PR has been merged. label Sep 27, 2022
@gabrieldonadel
gabrieldonadel deleted the feat/aspect-ratio-string-support branch September 27, 2022 12:06
@marklawlormarklawlor mentioned this pull request Oct 12, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Shared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@gabrieldonadel@analysis-bot@facebook-github-bot@cipolleschi@react-native-bot@necolas