feat: progressbar new way of handling classes#1607
feat: progressbar new way of handling classes#1607shinokada merged 2 commits intothemesberg:mainfrom
Conversation
|
@jjagielka is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes refactor class handling for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Progressbar
participant Theme
User->>Progressbar: Render with { class: className, classes }
Progressbar->>Theme: Apply base styles and merge with classes/className
Progressbar-->>User: Rendered progress bar with unified class handling
sequenceDiagram
participant User
participant Progressradial
participant Theme
User->>Progressradial: Render with { class: className, classes }
Progressradial->>Theme: Apply base styles and merge with classes/className
Progressradial-->>User: Rendered radial progress with unified class handling
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/routes/docs/components/progress.md (1)
158-158: Fix grammatical issue by adding the missing article.The sentence should include "the" before "prop" for better readability.
-Use `labelInsideDiv` prop to style your progressbar. +Use the `labelInsideDiv` prop to style your progressbar.🧰 Tools
🪛 LanguageTool
[uncategorized] ~158-~158: You might be missing the article “the” here.
Context: ...le" /> ``` ## Custom style UselabelInsideDivprop to style your progr...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/progress/Progressbar.svelte(3 hunks)src/lib/progress/Progressradial.svelte(5 hunks)src/lib/progress/theme.ts(2 hunks)src/lib/types.ts(9 hunks)src/routes/component-data/Progressbar.json(1 hunks)src/routes/component-data/Progressradial.json(1 hunks)src/routes/docs/components/progress.md(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/types.ts (2)
src/lib/toolbar/theme.ts (1)
ToolbarGroupVariants(87-87)src/lib/progress/theme.ts (2)
ProgressbarVariants(4-4)ProgressradialVariants(123-123)
🪛 LanguageTool
src/routes/docs/components/progress.md
[uncategorized] ~158-~158: You might be missing the article “the” here.
Context: ...le" /> ``` ## Custom style Use labelInsideDiv prop to style your progr...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (22)
src/routes/docs/components/progress.md (3)
165-170: LGTM! Documentation correctly reflects the new classes prop structure.The examples properly demonstrate the transition from individual class props to the unified
classesobject approach, which aligns with the PR objectives.
184-186: LGTM! Animation examples updated correctly.The animation examples correctly use the new
classes={{labelInsideDiv: "..."}}pattern instead of the deprecatedlabelInsideClassprop.
199-202: LGTM! Custom color examples follow the new pattern.The custom color examples correctly demonstrate the new
classesprop usage with different background colors.src/routes/component-data/Progressradial.json (2)
8-63: LGTM! Props structure correctly updated for the new classes approach.The JSON correctly reflects the removal of individual class props (
labelInsideClass,outsideSpanClass, etc.) and the addition of the unifiedclass: classNameandclassesprops, which aligns with the PR objectives.
5-5:Details
✅ Verification successful
Verify the updated type definition link.
The link has been updated to point to line 1259. Please ensure this accurately reflects the location of the
ProgressradialPropstype definition.
🏁 Script executed:
#!/bin/bash # Description: Verify the ProgressradialProps type definition location # Expected: The type should be found around line 1259 ast-grep --pattern 'export interface ProgressradialProps'Length of output: 59
🏁 Script executed:
#!/bin/bash # Locate the type definition for ProgressradialProps in src/lib/types.ts # Confirm the file exists if [[ ! -f src/lib/types.ts ]]; then echo "ERROR: src/lib/types.ts not found" exit 1 fi # Search with both casing variations rg -n 'ProgressradialProps' src/lib/types.ts || true rg -n 'ProgressRadialProps' src/lib/types.ts || trueLength of output: 233
Validated type definition link
The
ProgressradialPropsinterface is indeed exported at line 1259 ofsrc/lib/types.ts, so the GitHub URL is accurate. No further action needed.src/routes/component-data/Progressbar.json (2)
8-51: LGTM! Props structure correctly updated for the new classes approach.The JSON correctly reflects the removal of individual class props (
labelInsideClass,oustsideSpanClass, etc.) and the addition of the unifiedclass: classNameandclassesprops, maintaining consistency with the refactoring.
5-5:Details
✅ Verification successful
Verify the updated type definition link.
The link has been updated to point to line 1247. Please ensure this accurately reflects the location of the
ProgressbarPropstype definition.
🏁 Script executed:
#!/bin/bash # Description: Verify the ProgressbarProps type definition location # Expected: The type should be found around line 1247 ast-grep --pattern 'export interface ProgressbarProps'Length of output: 56
🏁 Script executed:
#!/bin/bash # Search for the ProgressbarProps definition or references in the codebase rg -n "ProgressbarProps" -C2 .Length of output: 3127
ProgressbarProps link verified
The link in
src/routes/component-data/Progressbar.jsoncorrectly points to line 1247 insrc/lib/types.tswhere theProgressbarPropsinterface is defined. No changes required.src/lib/progress/theme.ts (3)
1-1: LGTM! Import properly extended for variant types.Adding the
VariantPropsimport is necessary for the new type definitions that follow.
3-4: LGTM! Well-structured type definitions for Progressbar.The type definitions follow a good pattern:
ProgressbarClassesprovides type safety for the classes prop by referencing the theme slotsProgressbarVariantsproperly combines variant props with the classes object
122-123: LGTM! Consistent type definitions for Progressradial.The type definitions maintain the same pattern as Progressbar, ensuring consistency across components and proper type safety for the new classes prop structure.
src/lib/progress/Progressradial.svelte (6)
2-3: LGTM! Imports properly organized for the new approach.The imports are correctly updated to include the theme and types, and
twMergeis appropriately removed since it's no longer needed with the new classes structure.
8-8: LGTM! Props destructuring correctly implements the new classes approach.The destructuring properly includes the new
class: classNameandclassesprops while removing the individual class props, which aligns perfectly with the PR objectives.
38-41: LGTM! Outside label styling correctly uses the new classes structure.The implementation properly uses the theme functions with the classes object for
outsideDiv,outsideSpan, andoutsideProgressstyling.
44-44: LGTM! Base element styling properly combines all class sources.The base element correctly combines
size,classes?.base, andclassNameusingclsx, which provides a clean and flexible approach to class merging.
54-54: LGTM! Inside label styling follows the new pattern.The inside label correctly uses the
labelInsideDivfunction with the classes object for consistent styling.
79-80: LGTM! JSDoc comments updated to reflect the new props.The JSDoc properly documents the new
class: classNameandclassesprops, replacing the previous individual class props.src/lib/progress/Progressbar.svelte (3)
2-2: Good import organization.Moving the type import to the top improves code readability and follows best practices for import organization.
8-8: Excellent prop consolidation.The refactoring from multiple individual class props to a single
classesobject andclass: classNamesignificantly improves the component's API and reduces prop clutter.
46-60: Documentation accurately reflects the API changes.The component documentation has been properly updated to reflect the new consolidated class handling approach, removing references to old individual class props and adding the new
class: classNameandclassesprops.src/lib/types.ts (3)
1174-1174: Import supports the new variant type system.The addition of
ProgressbarVariantsandProgressradialVariantsimports properly supports the new consolidated class handling approach.
1247-1257: Excellent type safety improvement.Extending
ProgressbarPropswithProgressbarVariantsprovides proper type safety for the newclassesobject while maintaining compatibility with standard HTML div attributes.
1259-1272: Consistent type definition pattern.The
ProgressradialPropsinterface follows the same excellent pattern asProgressbarProps, ensuring consistency across the progress component family and proper type safety for the refactored class handling.
| </div> | ||
| {:else} | ||
| <div class={twMerge(insideDiv(), size, clsx(labelInsideClass))} style="width: {_progress.current}%"></div> | ||
| <div class={insideDiv({ class: clsx(classes?.labelInsideDiv, size) })} style="width: {_progress.current}%"></div> |
There was a problem hiding this comment.
Fix incorrect class reference.
There's a copy-paste error where classes?.labelInsideDiv is used instead of classes?.insideDiv. This will apply the wrong styling to the inside div element.
Apply this fix:
- <div class={insideDiv({ class: clsx(classes?.labelInsideDiv, size) })} style="width: {_progress.current}%"></div>
+ <div class={insideDiv({ class: clsx(classes?.insideDiv, size) })} style="width: {_progress.current}%"></div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class={insideDiv({ class: clsx(classes?.labelInsideDiv, size) })} style="width: {_progress.current}%"></div> | |
| <div class={insideDiv({ class: clsx(classes?.insideDiv, size) })} style="width: {_progress.current}%"></div> |
🤖 Prompt for AI Agents
In src/lib/progress/Progressbar.svelte at line 38, the class reference uses
classes?.labelInsideDiv instead of the correct classes?.insideDiv, causing
incorrect styling. Replace classes?.labelInsideDiv with classes?.insideDiv in
the clsx function call to apply the proper styles to the inside div element.
|
Can we use the following to set So that uses can use complex inputs. The following is not supposed to be practical, just as an example. If we can use this, then all class should be wrapped with |
|
Yes. That make sense. Good one. |
|
We need to get rid of |
|
What is your idea like Accordion.svelte that uses only one slot, Are you still planning to use |
|
Since this change will be BREAKING CHANGE that leads us to v2. How about we change class names for props that will be used to |
|
We also need to clean up transition related prop names. Some use |
|
@jjagielka I'm thinking this inplementation to other components will be a breaking change, we need to start using v2.0.0-next.1 branch/tag and commit future changes to this branch/tag. What do you think? |
|
Yes. agree. This is a big change that will cause a lot of changes at user's codes. We can use generic type in // Generic type definition for ComponentVariants
export type ComponentVariants<T extends ((...args: any) => any) & { slots: object }> = (
VariantProps<T> & { classes?: Partial<{ [K in keyof T["slots"]]: ClassValue }> }
);then in components it will be only: export type ProgressbarVariants = ComponentVariants<typeof progressbar>; |
2 participants
📑 Description
Proposal of the new prop
classesthat replaces the customization props like:divClass,insideLabelClass, etc.Now the component has a prop
classeswhich accept the object that is a record oftwVariantsslots names and overriding classes:This will reduce the number of props per element while increasing the type safety.
Status
✅ Checks
ℹ Additional Information
Summary by CodeRabbit
New Features
classesprop and a mainclassprop for styling, replacing multiple individual class-related props.Documentation
classesprop and revised styling approach for Progressbar and Progressradial components.Chores