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

Cleanup.Spinner #218

Open
wants to merge 30 commits into
base: 2.0.0-alpha1
Choose a base branch
from
Open

Cleanup.Spinner #218

wants to merge 30 commits into from

Conversation

SBBlee
Copy link

@SBBlee SBBlee commented May 22, 2024

Simplifies attribute naming conventions (removes data- prefix) as mentioned in issue #202

Capture3

maxatdetroit and others added 30 commits April 29, 2024 14:03
[RFC] Create table v2 to resolve performance issues
Add Close Button to Alert Component
@SBBlee SBBlee requested a review from maxatdetroit May 22, 2024 20:08
@SBBlee
Copy link
Author

SBBlee commented May 22, 2024

@maxatdetroit, I just wanted to clarify that #202 was the only issue that pertained to this component

@maxatdetroit
Copy link
Member

Yes, good clarification. That was correct when you asked, but I did notice while reviewing this PR that the storybook stories are setting missing controls to undefined in the code examples, so I added this request to #201:

Also, unless the attribute is a boolean attribute, the component should treat attribute="undefined" the same as omitting the attribute entirely. In Storybook stories, we should prefer omitting the attribute entirely if the storybook control is not set.

Would you please update the Storybook story for the spinner to omit attributes if they're not set in the story (e.g. size and display-type should be omitted from the component in the earlier screenshot)? Then, let's make sure the spinner still functions as expected when those attributes are omitted.

@maxatdetroit maxatdetroit changed the base branch from dev to 2.0.0-alpha1 May 23, 2024 14:40
@maxatdetroit maxatdetroit changed the base branch from 2.0.0-alpha1 to dev May 23, 2024 14:40
@maxatdetroit
Copy link
Member

maxatdetroit commented May 23, 2024

Also, all PRs for 201, 202, and 205 should be based on the 2.0.0-alpha1 branch. I failed to mention that the other day - sorry! You'll need to:

git rebase 2.0.0-alpha1

This is a case where a rebase is required instead of using merge. After you've done that and pushed the changes to this PR. We can update the base branch on the PR as well (using the edit button near the PR title):

image

@SBBlee SBBlee changed the base branch from dev to 2.0.0-alpha1 May 23, 2024 15:00
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.

I think we moved this implementation to #219 due to base branch issues on this PR. Is that right? Can we close this PR in favor of #219?

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.

4 participants