Skip to content

Add aspect ratio support #123

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 12, 2023
Merged

Add aspect ratio support #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/__tests__/aspectRatio.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import transformCss from '..'

it('handles regular aspect ratio values', () => {
expect(transformCss([['aspect-ratio', '1.5']])).toEqual({
aspectRatio: 1.5,
})
})

it('handles CSS-style aspect ratios', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually want to support the CSS-style aspect ratios? aspect-ratio isn't a CSS prop, so we're actually extending a React Native prop with a "CSS-style" syntax, so this is something that only works when using this parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there is a ratio type in CSS that's used in media queries (mdn), and this is shipped in browsers. This is the semantics this PR implements

There's also a proposal for a React Native style aspect-ratio property here. But that is just a proposal, it might never be implemented

I think we can be sure that if CSS did implement the proposal, it will use the ratio semantics. But this also isn't a feature anyone requested, so I don't mind if you think it's too premature to merge it in! I don't mind either way! 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

The property and the syntax is probably not going to change as it has a proposal, and the same syntax also being used in other CSS features, but there is a small chance that it will never implemented in browsers.

We can go forward with this, but it might mean that we have to do a breaking change in the future if things don't go as planned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I give it 50/50 of being implemented in browsers if that helps

Currently we allow aspect-ratio: 1.5 (via the generic value transformer), which is not web compatible.

We also have custom transforms for shadowOffset and textShadowOffset, and we indirectly support shadowColor and shadowRadius etc. via the generic value transformer. But these properties will almost certainly never be supported on the web

But I don't know if we should stop people using styles that RN added on-top of CSS

The values for some of these additional styles are objects, so to support them, we need a custom transformer. There's also no proposed CSS spec for this, so we're really hoping the web doesn't add these in a breaking way

What do you think on these?

Aspect-ratio is kind of interesting because the spec pretty much copies what RN does (except for the syntax). But the generic value transformer (i.e. 1.5) is not compatible with the proposed spec. The proposed spec only allows ratios like 3 / 2

So say browsers do support this, we would need to do a breaking change to remove the 1.5 version - and we'd need to do this regardless of if we decide to merge this PR

If we do decide to merge this in, I'll actually need to make this stricter - since I think you can put stuff like -3.2 / 8.4 - to avoid a breaking change later on

But if we decide not to merge this in (and while we're doing breaking changes with borders anyway), it might be an opportunity to reconsider the properties I put above

Copy link
Contributor

Choose a reason for hiding this comment

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

But I don't know if we should stop people using styles that RN added on-top of CSS

We should definitely not do that, but what I see as a bit of a risk is to add syntax that is not supported in RN, and is only a proposal in Web.

If people know that their app is only going to be used with RN, then they should be able to use the RN specific styling props.

Copy link
Contributor

Choose a reason for hiding this comment

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

So say browsers do support this, we would need to do a breaking change to remove the 1.5 version - and we'd need to do this regardless of if we decide to merge this PR

But currently aspect-ratio is a React Native-only property, so we could just think about it as something that the parser does not care about until it's implemented in CSS. When the CSS property is final, then we could allow both RN and Web way of giving the value, and people who want to render in both native/web could use 3/2 syntax.

What we could do is to add docs about which props are treated as CSS properties and which are considered as React Native specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Let's do that

I wanna find some time to work on documenting what we have, as a lot of people ask for this!

expect(transformCss([['aspect-ratio', '3 / 2']])).toEqual({
aspectRatio: 1.5,
})
})

it('handles CSS-style aspect ratios without spaces', () => {
expect(transformCss([['aspect-ratio', '3/2']])).toEqual({
aspectRatio: 1.5,
})
})

it('throws when omitting second value after slash', () => {
expect(() => transformCss([['aspect-ratio', '3/']])).toThrow()
})
12 changes: 12 additions & 0 deletions src/transforms/aspectRatio.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { NUMBER, SLASH } from '../tokenTypes'

export default tokenStream => {
let aspectRatio = tokenStream.expect(NUMBER)

if (tokenStream.hasTokens()) {
tokenStream.expect(SLASH)
aspectRatio /= tokenStream.expect(NUMBER)
}

return { aspectRatio }
}
2 changes: 2 additions & 0 deletions src/transforms/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
PERCENT,
AUTO,
} from '../tokenTypes'
import aspectRatio from './aspectRatio'
import border from './border'
import boxShadow from './boxShadow'
import flex from './flex'
Expand Down Expand Up @@ -53,6 +54,7 @@ const textShadowOffset = tokenStream => ({
})

export default {
aspectRatio,
background,
border,
borderColor,
Expand Down