-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -505,7 +505,10 @@ export default css` | |||
} | |||
|
|||
.button--loading sl-spinner { | |||
--indicator-color: currentColor; | |||
--track-color: rgb(147 153 158 / 35%); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 = ''; | |||
|
|||
/** |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
For
<sl-spinner>
size
property with optionssmall
,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)<sl-spinner>
style applied to loading buttonsOther fixes
<sl-tooltip>
border radius being applied<sl-dialog>