-
Notifications
You must be signed in to change notification settings - Fork 1
fix: improve preset handling & fix container module name #8
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
Conversation
✅ Deploy Preview for statuesque-boba-0fb888 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Pull Request Overview
This PR refactors preset handling and customization across the component library to streamline how presets are applied. The main improvements include:
- Centralized preset handling in
HtmlAtomcomponent with automatic class merging - Simplified component code by removing manual preset retrieval and class application
- Introduced a
$presetplaceholder system for flexible preset styling - Renamed
ModuleNametoPresetModuleNamefor clarity and consistency
Reviewed Changes
Copilot reviewed 130 out of 130 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/context/preset.svelte.ts | Renamed ModuleName to PresetModuleName and updated input.root to input |
| src/lib/context/index.ts | Exported PresetModuleName as ModuleName for backward compatibility |
| src/lib/components/atom/html-atom.svelte | Added preset handling logic with $preset placeholder and automatic class merging |
| src/lib/components/atom/types.ts | Added bond and preset props to HtmlAtomProps |
| src/lib/components/element/types.ts | Updated type annotations for better type safety |
| src/stories/Theme.svelte | Updated accordion preset configuration and removed unused popover.trigger preset |
| Multiple component files | Removed manual preset retrieval, simplified class application using $preset, and added bond prop |
| class: function () { | ||
| const bond = this; | ||
| return [bond.state.isActive ? 'text-foreground/100' : 'text-foreground/50']; | ||
| } |
Copilot
AI
Oct 22, 2025
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.
[nitpick] Using function() syntax and reassigning this to bond is unnecessarily verbose. The original arrow function with bond parameter is clearer and follows modern JavaScript best practices.
| class: function () { | |
| const bond = this; | |
| return [bond.state.isActive ? 'text-foreground/100' : 'text-foreground/50']; | |
| } | |
| class: (bond) => [ | |
| bond.state.isActive ? 'text-foreground/100' : 'text-foreground/50' | |
| ] |
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.
Using an arrow function, you cannot retrieve the value assigned to this arg.
| class={[ | ||
| 'h-full w-full flex-1 bg-transparent px-2 leading-1 outline-none', | ||
| toClassValue.bind(bond, preset?.class), | ||
| toClassValue.bind(bond, klass) |
Copilot
AI
Oct 22, 2025
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.
Using .bind() instead of .apply() changes the behavior - .bind() returns a new function rather than calling it immediately. This will result in functions being added to the class array instead of resolved class values.
| toClassValue.bind(bond, klass) | |
| toClassValue(bond, klass) |
| enter={scale} | ||
| enterConfig={{ duration: DURATION.fast, easing: circOut, start: 0.6 }} |
Copilot
AI
Oct 22, 2025
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.
The transition was changed from transition:scale to enter={scale} with a separate enterConfig. Verify that the enter prop with enterConfig provides the same functionality as the original transition:scale directive, as these are different APIs.
| let { | ||
| class: klass = '', | ||
| as = preset?.as ?? 'div', |
Copilot
AI
Oct 22, 2025
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.
The preset variable is used before it's defined. The line const preset = getPreset('combobox.item'); was removed but preset is still referenced, which will cause a runtime error.
| if (item) { | ||
| return item; | ||
| return item as DropdownItemBond; |
Copilot
AI
Oct 22, 2025
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.
Added type assertion as DropdownItemBond suggests a potential type mismatch. Review the type definitions to ensure this cast is necessary and safe.
| }); | ||
| } | ||
| return [preset?.class ?? '', '$preset', klass].map((arg) => toClassValue.apply(bond, [arg])); |
Copilot
AI
Oct 22, 2025
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.
When no $preset placeholder is found, this code adds the preset class, a literal '$preset' string, and the custom class. The literal '$preset' string in the class array will be treated as a CSS class name, which is incorrect.
| return [preset?.class ?? '', '$preset', klass].map((arg) => toClassValue.apply(bond, [arg])); | |
| const presetClass = preset?.class ?? ''; | |
| const customClasses = Array.isArray(klass) ? klass : [klass]; | |
| return [presetClass, ...customClasses].map((arg) => toClassValue.apply(bond, [arg])); |
| '$preset', | ||
| klass |
Copilot
AI
Oct 22, 2025
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.
Missing {bond} prop even though the preset system now requires it. This inconsistency exists across multiple drawer components and could lead to preset not being properly resolved.
| toClassValue.apply(bond, [preset?.class]), | ||
| toClassValue.apply(bond, [klass]) | ||
| ]} | ||
| class={['pointer-events-none absolute inset-0', '$preset', klass]} |
Copilot
AI
Oct 22, 2025
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.
Missing preset and {bond} props. Without the preset prop, the $preset placeholder won't be replaced with actual preset classes.
| class={['pointer-events-none absolute inset-0', '$preset', klass]} | |
| class={['pointer-events-none absolute inset-0', preset, klass]} |
| {base} | ||
| class={[toClassValue.apply(bond, [preset?.class]), toClassValue.apply(bond, [klass])]} | ||
| {bond} | ||
| class={['$preset', klass]} |
Copilot
AI
Oct 22, 2025
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.
Missing preset prop specification. The $preset placeholder is used but no preset key is provided to resolve it.
| <div class="bg-foreground h-80 flex-1 rounded-lg"></div> | ||
| {/each} | ||
| </div> | ||
| <ContainerCmp class="flex flex-col items-center gap-4 w-full"> |
Copilot
AI
Oct 22, 2025
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.
Corrected spelling of 'contaiener' to 'container' in the directory name would be consistent, but this change only updates the story file content.
No description provided.