Fix/removing NaN for shadow

rahulchaudhary2244 commented 1 month ago

Pull Request Template

Description

This PR addresses the issue where clearing the Blur property in the Shadow settings results in "NaN" being displayed on the UI. It is essential to handle this case appropriately to improve the user experience and ensure that "NaN" is not shown to the users.

Closes https://github.com/suryanshsingh2001/mockly/issues/23

Changes Made

Updated the ShadowManager component to validate and set default values for shadowX, shadowY, and blur properties on blur events.

Type of Change

[x] Bug fix (non-breaking change that fixes an issue)
[ ] New feature (non-breaking change that adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation update

Screenshots

How Has This Been Tested?

Manually tested on local environment

Checklist

Check off the following items before submitting the pull request:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my code
[x] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] New and existing unit tests pass locally with my changes
[ ] Any dependent changes have been merged and published in downstream modules

vercel[bot] commented 1 month ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mockly	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 3, 2024 8:30am

rahulchaudhary2244 commented 1 month ago

Hi @suryanshsingh2001 , Could you please take a look at my PR when you have some time? Also, it would be great if you could add the hacktoberfest-accepted label to ensure it gets counted for the event.

rahulchaudhary2244 commented 1 month ago

Hey, I reviewed the code, and it looks like this is already handled— the minimum value for the border radius is set to not go below 0.

suryanshsingh2001 commented 1 month ago

Hey, I reviewed the code, and it looks like this is already handled— the minimum value for the border radius is set to not go below 0.

https://github.com/user-attachments/assets/e3e61aed-5de3-4a96-b352-513a02dc798c

@rahulchaudhary2244 Please have a look, using the arrows still allowing negative values. But, yes otherwise it defaults to zero.

rahulchaudhary2244 commented 1 month ago

Oh, I see now! You were referring to the Blur value—my mistake, I thought you meant border-radius. You're right, this needs to be fixed. I'll take care of it.

rahulchaudhary2244 commented 1 month ago

Hi @suryanshsingh2001 , this should be good now.

suryanshsingh2001 commented 1 month ago

Oh, I see now! You were referring to the Blur value—my mistake, I thought you meant border-radius. You're right, this needs to be fixed. I'll take care of it.

My apologies for the confusion 😅!

rahulchaudhary2244 commented 1 month ago

@suryanshsingh2001 , I believe we should introduce unit tests or integration tests to prevent issues like these edge cases from slipping through in the future. What do you think ?

suryanshsingh2001 commented 1 month ago

@suryanshsingh2001 , I believe we should introduce unit tests to prevent issues like these edge cases from slipping through in the future. What do you think ?

You are right; however, I’m not familiar with a lot of testing. Please let me know how to approach this.

suryanshsingh2001 / mockly