-
-
Notifications
You must be signed in to change notification settings - Fork 321
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
Made ProgressRing show label on demand #2996
base: next
Are you sure you want to change the base?
Made ProgressRing show label on demand #2996
Conversation
🦋 Changeset detectedLatest commit: a5c8f78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@phamduylong is attempting to deploy a commit to the Skeleton Labs Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 Long. If you could make these couple minor changes and supply the documentation updates requested, then this should be good to go.
packages/skeleton-react/src/lib/components/ProgressRing/ProgressRing.tsx
Outdated
Show resolved
Hide resolved
packages/skeleton-react/src/lib/components/ProgressRing/types.ts
Outdated
Show resolved
Hide resolved
Yeah it's not finished yet, there's no draft flag on PR in GitHub it seems. I'll probably get it done soon enough when I figure out how to update site examples and refactor the stuff you pointed out. Thanks for the review. |
@endigo9740 probably could be permission issues that caused this :D I don't have the option. Could you un-draft this and give this another review? Edit: NVM I can un-draft it. This is ready nonetheless |
|
||
<div class="grid grid-cols-1 gap-4 justify-center"> | ||
<ProgressRing {value} {max} showLabel /> | ||
<input type="range" class="input max-w-32" bind:value {max} /> |
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 think we go ahead and drop the range input for the label example. The focus should be the label itself. The first demo already shows you can bind the and manipulate the state like this.
Less code = easier for folks to digest!
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.
@phamduylong if you can implement this change I think we're good to go!
Linked Issue
Closes #2992
Description
Made label show only on demand
TODO: update site examples and generate changesets
Changsets
Instructions: Changesets automate our changelog. If you modify files in
/packages
, runpnpm changeset
in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should beminor
while chores and bugfixes should bepatch
. Please prefix the changeset message withfeat:
,bugfix:
orchore:
.Checklist
Please read and apply all contribution requirements.
dev
branch (NEVERmaster
)docs/
,feat/
,chore/
,bugfix/
pnpm ci:check
pnpm format
pnpm test