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

Clean-up.Spinner #219

Merged
merged 2 commits into from
Aug 5, 2024
Merged

Clean-up.Spinner #219

merged 2 commits into from
Aug 5, 2024

Conversation

SBBlee
Copy link

@SBBlee SBBlee commented May 24, 2024

Removes data prefix and omits unused attributes as mentioned in issue #201 and #202

Capture

@SBBlee SBBlee requested a review from maxatdetroit May 24, 2024 14:16
Copy link
Member

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

Just a minor comment regarding the storybook attribute implementation. Please address then I'll merge this in.

Comment on lines 36 to 38
} else {
delete spinner.size;
}
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary because the size attribute will never have been set if we reach this else condition.

Copy link
Member

Choose a reason for hiding this comment

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

Removing this else statement entirely should continue to work as expected.

Comment on lines 41 to 43
} else {
delete spinner.displayType;
}
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment regarding unnecessary delete statement.

@maxatdetroit maxatdetroit mentioned this pull request Jul 31, 2024
@maxatdetroit maxatdetroit self-requested a review July 31, 2024 17:37
Copy link
Member

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

Requesting changes so we can fix the lingering comments from the previous review.

Copy link
Member

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

Thanks for making those tweaks.

@maxatdetroit maxatdetroit merged commit e1cfb68 into 2.0.0-alpha1 Aug 5, 2024
1 check passed
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