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:revolving dot issue #140 #141

Merged
merged 6 commits into from
Sep 8, 2022
Merged

Conversation

Atishay-J
Copy link
Contributor

This fixes Revolving dot issue #140

Both the circles are now scaling up and down properly without getting cropped.

revolving fix

Some additions

  • Added support for secondary color
  • Added support to change stroke width

@Atishay-J
Copy link
Contributor Author

Atishay-J commented Sep 3, 2022

I am not sure about what package version should I give it.
while this change is backward compatible, this also makes the circle radius comparatively smaller than the current version.
users might need to change the radius to a bigger number.

@mhnpd
Copy link
Owner

mhnpd commented Sep 6, 2022

@Atishay-J Let's update the props type of Radius and the Stroke Width for this spinner to number? I think, we can not just support any number range for such value. If you have played around with configuration let's limit it with the type check. i.e: radius: 4 | 5 | 6 | 7 | 8 |. At least user will get a console warn if they tries to pass radius value outside of desired range. I think this is an issue with other spinner as well.

Please feel free to open a PR for other spinner if you like.

@@ -5,13 +5,14 @@ import { BaseProps, DEFAULT_COLOR, DEFAULT_WAI_ARIA_ATTRIBUTE } from '../type'
interface RevolvingDotProps extends BaseProps {
radius?: string | number
secondaryColor?: string
strokeWidth?: string | number
Copy link
Owner

Choose a reason for hiding this comment

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

Rethink about the props type for the spinner. Number will make more sense and we can limit it to supported range.

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 agree, we can keep it a number.
But I think we should not have a limit to it, people may want to have granular control for the component.

In this case they can mix and match radius and strokeWidth according to their preference

@mhnpd
Copy link
Owner

mhnpd commented Sep 6, 2022

I am not sure about what package version should I give it. while this change is backward compatible, this also makes the circle radius comparatively smaller than the current version. users might need to change the radius to a bigger number.

I think it should be a major update, existing spinner would change its looks and feel silently.

@Atishay-J Atishay-J requested a review from mhnpd September 7, 2022 16:01
@mhnpd mhnpd merged commit 906a275 into mhnpd:master Sep 8, 2022
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