phamfoo / react-native-figma-squircle

Figma-flavored squircles for React Native
MIT License
318 stars 15 forks source link

fix: added width, height props in Svg component #10

Closed TumenbayarSh closed 2 years ago

TumenbayarSh commented 2 years ago

Squircle View is not showing. Added svg properties

alevkov commented 2 years ago

@TumenbayarSh your changes break the component for me. I'm seeing this error on render:

Render error
null is not an object (evaluating 'squircleSize.width`)
alevkov commented 2 years ago

@TumenbayarSh you're not guarding against squircleSize being null on initial render. I think this is what you meant:

<Svg
  width={squircleSize?.width}
  height={squircleSize?.height}
  viewBox={`0 0 ${squircleSize?.width ?? 0} ${squircleSize?.height ?? 0}`}
  fill="none"
>
phamfoo commented 2 years ago

Squircle View is not showing

Thanks @TumenbayarSh. The change looks good, I haven't run into this issue though. When does this happen?

alevkov commented 2 years ago

@tienphaw please check out my suggestions. @TumenbayarSh 's changes crash the component.

TumenbayarSh commented 2 years ago

@tienphaw please check out my suggestions. @TumenbayarSh 's changes crash the component.

Thanks for reviewing. I'll check it

TumenbayarSh commented 2 years ago

@TumenbayarSh you're not guarding against squircleSize being null on initial render. I think this is what you meant:

<Svg
  width={squircleSize?.width}
  height={squircleSize?.height}
  viewBox={`0 0 ${squircleSize?.width ?? 0} ${squircleSize?.height ?? 0}`}
  fill="none"
>

Thanks @alevkov . I'll check null option

phamfoo commented 2 years ago

@TumenbayarSh can you take a look at my comments? I think I'd be nicer if we just render null instead of the Svg element when squircleSize is not available. That way we can get rid of all the null checks.

phamfoo commented 2 years ago

I added explicit width and height here https://github.com/tienphaw/react-native-figma-squircle/commit/f607aa0eda377aa85298a76ae5f4611b1c0d14f4. I didn't use the viewBox prop since we don't need the SVG to scale. Also, I couldn't reproduce the issue. If you're still experiencing the issue in the latest release, it'd be really helpful if you could provide a minimal repro.