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

Updates to the <sl-spinner> component #28

Merged
merged 7 commits into from
Jan 24, 2024
Merged

Conversation

slhinyc
Copy link

@slhinyc slhinyc commented Jan 23, 2024

For <sl-spinner>

  • Update styling & animation
  • Add size property with options small, medium, large, x-large, custom
  • custom option is the default, and keeps the option to apply any size to the spinner (so that these updates don't break current uses of the spinner)
  • Update doc site examples page for the spinner
  • Update <sl-spinner> style applied to loading buttons
  • Update loading button examples to show different buttons sizes

Other fixes

  • Update wrong <sl-tooltip> border radius being applied
  • Delete extraneous unused property previously added to <sl-dialog>

@slhinyc slhinyc requested a review from a team January 23, 2024 22:41
@slhinyc slhinyc requested a review from CrookedGrin as a code owner January 23, 2024 22:41
Copy link

vercel bot commented Jan 23, 2024

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

Name Status Preview Comments Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 24, 2024 3:33pm

@@ -505,7 +505,10 @@ export default css`
}

.button--loading sl-spinner {
--indicator-color: currentColor;
--track-color: rgb(147 153 158 / 35%);

Choose a reason for hiding this comment

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

I don't love hard-coding this color in here. Is there a way we can use a css variable instead?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, right! I forgot that we now have rgb variables for all the colors. I fixed to use the variable. TY for the callout!

@@ -103,12 +103,6 @@ export default class SlDialog extends ShoelaceElement {
*/
@property({ reflect: true }) label = '';

/**

Choose a reason for hiding this comment

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

Why is this being removed? And does this change have anything to do with the spinner? If not, we should probably leave it out of this PR.

Copy link
Author

Choose a reason for hiding this comment

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

It's a mistake I made from a previous PR (the one for the recent huge update). The property doesn't actually exist -- you'll see that the description is identical to another property that does exist, because I'd copy-pasta-ed it originally. When I updated the Dialog component before, I was going to add a new property for the header icon & created that line, but later realized that it made more sense as a slot and didn't need to be a new property. But I forgot to delete this line that I'd added. I only saw it later & realized that I forgot to delete it! It's such a small change that I thought it would be okay to add here, but am happy to move this bit to another PR if you prefer.

Choose a reason for hiding this comment

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

Gotcha! No, in that case it's fine to slide in here.

Copy link

@CrookedGrin CrookedGrin left a comment

Choose a reason for hiding this comment

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

Looks great, slick CSS!

@slhinyc slhinyc merged commit 6c3fc4b into next Jan 24, 2024
4 checks 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