Skip to content

borderRadius property regression? #11

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

Closed
jh3y opened this issue Nov 24, 2016 · 23 comments
Closed

borderRadius property regression? #11

jh3y opened this issue Nov 24, 2016 · 23 comments

Comments

@jh3y
Copy link

jh3y commented Nov 24, 2016

I had opened up an issue over on styled-components here about the borderRadius property on native.

It seems going from version 1.0.11 to 1.1.1 in styled-components regressed how borderRadius would work.

borderTopLeftRadius etc. seemingly have no effect when developing for native from what I can tell in the simulator and what works is simply using borderRadius.

After having a dive into the source, I think this just requires a slight change to the grammar file to only output borderRadius.

I'll submit a PR for this.

@jh3y

@jacobp100
Copy link
Contributor

I'll have to test this. RN claims to support this. https://facebook.github.io/react-native/docs/view.html#style

@mxstbr
Copy link
Member

mxstbr commented Nov 24, 2016

I don't quite understand what the issue here is, what should be happening that isn't happening?

@jacobp100
Copy link
Contributor

I’m not able to reproduce this on 0.38. You are correct in how border-radius splats into borter-{top,bottom}-{left,right}-radius, but that does seem to work.

screen shot 2016-11-24 at 10 08 22
screen shot 2016-11-24 at 10 17 41

@jh3y
Copy link
Author

jh3y commented Nov 24, 2016

I've just tested this out again using react-native@0.38 and styled-components@1.1.1, same issue.

In the simulator, could you inspect that with the inspector and see what style properties are given?

What is cssta(View)? I'm using styled.Image to get this issue. With a View, you will get the correct borderRadius set but when you apply border radius to an Image it is ignored.

That's because rn needs the borderRadius prop for an Image it seems.

The way styled-components previously worked meant that the borderRadius property was set as expected and the Image itself was round so you could do as shown here but with styled-components

<RoundImage source={{uri: 'http://placehold.it/100x100'}}/>
// styles
import styled from 'styled-components/native'
const RoundImage = styled.Image`
    height: 100;
    border-radius: 50;
    width: 100;
`

This either means not being able to upgrade to the newer version or having to refactor everywhere in a project where an Image is round to be wrapped by a View with overflow: hidden set.

I've just given this another go without styled-components at all and although Image also supports the borderRadius props the same as View, it seems that it doesn't respect them and currently only respects borderRadius on its own for Image.

So I've gone from styled-components -> css-to-react-native -> to react-native 😂 I'll have raise an issue over there I guess.

@jh3y

@jh3y jh3y changed the title borderRadius property regression borderRadius property regression? Nov 24, 2016
@jacobp100
Copy link
Contributor

Got you. It’s fixed in #13.

@jh3y
Copy link
Author

jh3y commented Nov 24, 2016

I don't think it can be fixed here can it? I think this is potentially a react-native issue.

borderTopLeftRadius etc. work differently with Image to how they do with View, as in they don't 😂

It would be a nice to have for now if only supplying one value, it only output borderRadius though 😎

border-radius: 10 -> borderRadius: 10

border-radius: 10 20 -> borderTopLeftRadius: 10, borderTopRightRadius: 20, etc.

Of course, if multiple props are supplied you can't really give a single value for borderRadius.

@jh3y

@jacobp100
Copy link
Contributor

jacobp100 commented Nov 24, 2016

Can you not wrap the image in a View, and use overflow: hidden?

Although we might need to look at some special casing.

@jh3y
Copy link
Author

jh3y commented Nov 24, 2016

Yeah I mentioned that as a solution above but then that means refactoring every instance of an Image component that is rounded to be wrapped by a View which isn't ideal overhead if it works how I intend when using styled-components@1.0.11 😕

Image respects borderRadius. It just doesn't respect the specifics.

That's why I'm leaning towards this being an issue with react-native. However, that being said, upgrading to the newer styled-components highlighted it for me because it doesn't generate the borderRadius property anymore and only the specifics.

@jacobp100
Copy link
Contributor

I’m thinking, special casing is going to be hard:

const StyledImage = styled(Image)`...`; // This is hard enough to work out that we need to special case
const SuperSpecialStyledImage = styled(StyledImage)`...` // This is almost impossible

Maybe we need to turn off this shorthand for the time being. @mxstbr

@mxstbr
Copy link
Member

mxstbr commented Nov 24, 2016

Let's turn it off then!

@mxstbr
Copy link
Member

mxstbr commented Nov 24, 2016

But that's a major version release again, ugh

@jacobp100
Copy link
Contributor

That's true. Maybe we just document the behaviour?

@jacobp100
Copy link
Contributor

Although I suppose this was a breaking change in the first place? I believe semver says we should revert the breaking change.

@mxstbr
Copy link
Member

mxstbr commented Nov 24, 2016

Oh if it was a breaking change in the first place let's push this as a patch. Would you mind doing that?

@jh3y
Copy link
Author

jh3y commented Nov 24, 2016

Happy to help if required 😄

@jacobp100
Copy link
Contributor

jacobp100 commented Nov 24, 2016

Over here! styled-components/styled-components#254

@jacobp100
Copy link
Contributor

Looks like we could turn this off soon

facebook/react-native@66a2940

There's a few other cases that'll get fixed too

@magik-chorne
Copy link

Poking on this one @jacobp100

@jacobp100
Copy link
Contributor

Has anyone confirmed this works now? And from what version it works

@jacobp100
Copy link
Contributor

Also, the change to re-enable these has to happen in styled-components. We take a blacklist of properties, and the border-radius is passed in there in SC

Once SC removes this, we'll remove the code around blacklisting here too, since it will no longer be needed

@jacobp100
Copy link
Contributor

@jacobp100
Copy link
Contributor

I'm closing this because it's been years since this was an issue

@fershibli
Copy link

fershibli commented Nov 30, 2023

I'm still having problem with it

My current setup:
react-native 0.72.6
styled-components: 6.1.0

Edit:
For future reference (for anyone around like me struggling to understand), I made it work by wrapping my Image inside a VIew, like this:

ImageWrapperView: styled(View)`
    overflow: hidden;
    width: 280px;
    height: 328px;
    border-radius: 20px;
`,
Image: styled(Image)`
    width: 100%;
    height: 100%;
`,

But it would be nice to rely directly on the styled Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants