Skip to content
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

fix(topRightBorder): fixed shadow on topRightCorner #33

Merged
merged 2 commits into from
Jan 16, 2022

Conversation

timqha
Copy link
Contributor

@timqha timqha commented Jan 14, 2022

Hi, When I used react-native-shdow-2 library, the right corner was missing.
I added a small fix, I hope you can check it.

@ftzi
Copy link
Owner

ftzi commented Jan 14, 2022

Hi @timqha, thanks for the report and PR.

Can you make a reproducible example of your issue in https://snack.expo.dev?

I never had an issue with it nor had a report about it, and I haven't touched that specific code for months.

The lib Snack demo is working fine: https://snack.expo.dev/@srbrahma/react-native-shadow-2-sandbox

Not saying you are wrong, but I need to know at what conditions this is happening.

@ftzi
Copy link
Owner

ftzi commented Jan 14, 2022

Just found an issue in the lib code:

I was looking at the bottomRight corner, as both should use the same horizontal positioning where you did the change:

https://github.com/SrBrahma/react-native-shadow-2/blob/8da90f745d96328dac38c2e6d01a225c24043ecd/src/index.tsx#L397-L402

https://github.com/SrBrahma/react-native-shadow-2/blob/8da90f745d96328dac38c2e6d01a225c24043ecd/src/index.tsx#L373-L378

both left styles uses just width. But, I took a second look and topRight is wrongly using bottomRight as translateX value.

This is probably the original cause of your issue. I would still like a repro of your issue so we can be sure that this typo was the issue and proper fix it. You may also just test the typo fix from bottomRight to topRight in your code and see if it fixes your issue. You can then fix your PR and I can merge it.

@timqha
Copy link
Contributor Author

timqha commented Jan 16, 2022

Hey, Thanks for checking it.
i can't reproduce in expo.
https://snack.expo.dev/@timqha/react-native-shadow-2-sandbox
Yes, I tried to reproduce this error in snack.expo.
But while doing so, I also tried doing the same in New Project (
npx react-native init MyProj --template react-native-template-typescript && yarn add react-native-shadow-2
image

I hope you have some ideas (this error is only reproducible on React-native in expo is not reproducible)

It was using bottomRight instead of topRight in translateX value
@ftzi
Copy link
Owner

ftzi commented Jan 16, 2022

But, I took a second look and topRight is wrongly using bottomRight as translateX value.

This is probably the original cause of your issue. I would still like a repro of your issue so we can be sure that this typo was the issue and proper fix it. You may also just test the typo fix from bottomRight to topRight in your code and see if it fixes your issue. You can then fix your PR and I can merge it.

This is surely what is happening.

You are using in your example topRightRadius: 10 but not bottomRightRadius, so bottomRightRadius is 0. The translateX value for topRight is, as I found out and said above, using wrongly bottomRightRadius instead of topRightRadius, so it's translating 0 instead of 10.

As distance defaults to 10, it happened that in your proposed fix of left: width-distance to work, but just by coincidence of values in your specific case, as I was expecting it to be.

For quickness, I've edited your change to properly fix this issue. I will then merge and update the package.

Thanks for the bug report!

@ftzi ftzi closed this Jan 16, 2022
@ftzi ftzi reopened this Jan 16, 2022
@ftzi ftzi merged commit ddb6a5e into ftzi:main Jan 16, 2022
@ftzi
Copy link
Owner

ftzi commented Jan 16, 2022

Just deployed it as 6.0.1. In a few moments it will be available to be updated using your package manager.

Tell me if it worked!

Once again, thanks for finding it!

@timqha
Copy link
Contributor Author

timqha commented Jan 17, 2022

thanks for the corrections, I really missed this point.
Thanks, It's work!

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

Successfully merging this pull request may close these issues.

2 participants