Conversation
docs: command-palette
|
@shinokada is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a new keyboard-navigable Command Palette component (Svelte) with theme variants and types, exposes it via public exports, includes a docs page and demo, and adds an end-to-end test asserting the demo page heading. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DemoPage as Demo Page
participant CP as CommandPalette
participant List as ItemList
User->>DemoPage: press Ctrl/Cmd+K or click toggle
DemoPage->>CP: set open = true
activate CP
CP->>CP: focus input
User->>CP: type query
CP->>CP: filter items (label/description/keywords)
CP->>List: render filtered items
alt Navigate (Arrow / j / k)
User->>CP: press navigation key
CP->>CP: update selectedIndex & scrollIntoView
CP->>List: highlight selected item
end
User->>List: click item or press Enter
List->>CP: select item
CP->>CP: call item.onselect()
CP->>DemoPage: call onclose()
CP->>CP: set open = false
deactivate CP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
e2e/extend.spec.ts(1 hunks)src/lib/command-palette/CommandPalette.svelte(1 hunks)src/lib/command-palette/index.ts(1 hunks)src/lib/command-palette/theme.ts(1 hunks)src/lib/index.ts(1 hunks)src/lib/types.ts(1 hunks)src/routes/docs-examples/extend/command-palette/+page.svelte(1 hunks)src/routes/docs/extend/command-palette.md(1 hunks)
🧰 Additional context used
🪛 ESLint
src/routes/docs-examples/extend/command-palette/+page.svelte
[error] 38-38: Unexpected goto() call without resolve().
(svelte/no-navigation-without-resolve)
[error] 70-70: Unexpected goto() call without resolve().
(svelte/no-navigation-without-resolve)
[error] 156-179: Each block should have a key
(svelte/require-each-key)
[error] 169-173: Each block should have a key
(svelte/require-each-key)
src/lib/command-palette/CommandPalette.svelte
[error] 139-139: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
|
Why don't you use |
…e to respect the app base, Add keys to each blocks
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/docs-examples/extend/command-palette/+page.svelte (1)
160-183: LGTM! Previous lint issues resolved.The
#eachblocks now include proper keys (command.idandkeyword), resolving the previoussvelte/require-each-keylint errors. The DOM reconciliation will now be predictable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/command-palette/CommandPalette.svelte(1 hunks)src/routes/docs-examples/extend/command-palette/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/command-palette/CommandPalette.svelte
🧰 Additional context used
🪛 ESLint
src/routes/docs-examples/extend/command-palette/+page.svelte
[error] 39-39: Unexpected goto() call without resolve().
(svelte/no-navigation-without-resolve)
[error] 73-73: Unexpected goto() call without resolve().
(svelte/no-navigation-without-resolve)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (2)
src/routes/docs-examples/extend/command-palette/+page.svelte (2)
1-6: LGTM! Clean imports and state setup.The imports are appropriate and the use of Svelte 5's
$staterune for reactivity is correct.
189-189: LGTM! CommandPalette properly configured.The component binding and props are correctly set up with
bind:openfor reactivity, a custom shortcut key, and vim mode enabled.
|
I tried it before but it didn't work. For now, I use div. We can change it later. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/command-palette/theme.ts (1)
35-42: Consider removing the unusedselectedvariant.The
selectedvariant (lines 36-39) appears unused since theitemslot already handles selection viaaria-selected:pseudo-classes (line 22). Having both could be confusing.If the variant is not needed, apply this diff:
- variants: { - selected: { - true: "bg-primary-600 text-white", - false: "text-gray-900 dark:text-gray-100" - } - }, - - defaultVariants: {} + variants: {}, + defaultVariants: {}Alternatively, if you plan to use the variant for programmatic styling, keep it but add a comment explaining its purpose.
src/lib/types.ts (1)
2178-2185: Consider clarifying theiconfield type.The
icon?: stringfield could benefit from more specific typing or documentation to clarify whether it represents an icon name, URL, SVG string, or component identifier.If it's meant to be an icon identifier from a specific icon library, consider:
export interface CommandItem { id: string; label: string; description?: string; icon?: string; // e.g., heroicon name or custom icon identifier keywords?: string[]; onselect: () => void; }Alternatively, if you want more flexibility:
export interface CommandItem { id: string; label: string; description?: string; icon?: string | Component; // Allow icon name or Svelte component keywords?: string[]; onselect: () => void; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/command-palette/CommandPalette.svelte(1 hunks)src/lib/command-palette/theme.ts(1 hunks)src/lib/theme/themes.ts(1 hunks)src/lib/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/command-palette/CommandPalette.svelte
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/command-palette/theme.ts (1)
src/lib/theme/themeUtils.ts (1)
Classes(12-14)
src/lib/types.ts (1)
src/lib/command-palette/theme.ts (1)
CommandPaletteVariants(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (5)
src/lib/theme/themes.ts (1)
81-81: LGTM!The export follows the established pattern and is correctly placed in the extend section.
src/lib/command-palette/theme.ts (2)
1-4: LGTM!The imports and type definition follow the established pattern used throughout the codebase.
6-43: Verify modal accessibility implementation.Based on the PR comments, this component uses a div with backdrop instead of the Dialog component. Ensure the implementation includes essential modal patterns: focus trap, Escape key handling, and proper ARIA attributes (role="dialog", aria-modal="true", aria-labelledby, etc.).
You can verify accessibility by checking if the CommandPalette component implementation includes:
- Focus trap that keeps keyboard navigation within the modal
- Escape key to close
- Proper ARIA roles and attributes
- Return focus to trigger element on close
If these are missing, consider revisiting the Dialog component approach or implementing these patterns manually.
src/lib/types.ts (2)
120-120: LGTM!The import follows the established pattern and is correctly placed with other extend component imports.
2187-2195: LGTM!The interface is well-typed with appropriate optional fields and callbacks. It extends the correct base types and provides good flexibility for component usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/types.ts (1)
2178-2185: Consider clarifying the icon type.The
icon?: string | Componenttype is flexible but potentially ambiguous. If the string variant represents an icon name, CSS class, or path, consider using a more specific type or adding a JSDoc comment to clarify the expected format.Example:
/** * Icon can be a Svelte Component or a string representing an icon class/name */ icon?: string | Component;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/command-palette/theme.ts(1 hunks)src/lib/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/command-palette/theme.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/types.ts (1)
src/lib/command-palette/theme.ts (1)
CommandPaletteVariants(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (2)
src/lib/types.ts (2)
120-120: LGTM!The import follows the established pattern for variant types in this file.
2187-2195: LGTM!The interface follows established patterns in the codebase. The extension of
HTMLAttributes<HTMLDivElement>is appropriate given the implementation approach mentioned in the PR discussion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/types.ts (1)
2177-2188: Consider simplifying the JSDoc comment formatting.The interface structure looks solid with appropriate required and optional fields. The flexibility of
iconbeing either a string or Component is good for different use cases.For consistency with other single-line comments in this file, consider simplifying the JSDoc:
description?: string; - /** - * Icon can be a Svelte Component or a string representing an icon class/name - */ + /** Icon can be a Svelte Component or a string representing an icon class/name */ icon?: string | Component;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/types.ts(9 hunks)src/routes/docs-examples/extend/command-palette/+page.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/routes/docs-examples/extend/command-palette/+page.svelte
🧬 Code graph analysis (1)
src/lib/types.ts (6)
src/lib/drawer/theme.ts (1)
DrawerHandleVariants(78-78)src/lib/forms/helper/theme.ts (1)
HelperVariants(4-4)src/lib/toolbar/theme.ts (1)
ToolbarGroupVariants(6-6)src/lib/skeleton/theme.ts (2)
TestimonialPlaceholderVariants(9-9)WidgetPlaceholderVariants(12-12)src/lib/tooltip/theme.ts (1)
TooltipVariants(4-4)src/lib/command-palette/theme.ts (1)
CommandPaletteVariants(4-4)
🪛 ESLint
src/routes/docs-examples/extend/command-palette/+page.svelte
[error] 43-43: Unexpected goto() call without resolve().
(svelte/no-navigation-without-resolve)
[error] 75-75: Unexpected goto() call without resolve().
(svelte/no-navigation-without-resolve)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (3)
src/routes/docs-examples/extend/command-palette/+page.svelte (1)
136-189: LGTM!The template markup is well-structured with proper responsive layout, keyboard shortcut instructions, and a demo grid showing all available commands. The
{#each}blocks correctly use keys (command.idandkeyword), and the CommandPalette component binding and props look appropriate.src/lib/types.ts (2)
120-120: LGTM!The import of
CommandPaletteVariantsfollows the established pattern for importing component variants in this file.
2190-2198: LGTM!The
CommandPalettePropsinterface follows the established pattern in this file by extending both component variants and HTML attributes. All props are appropriately typed and optional, providing good flexibility for component usage.
| @@ -0,0 +1,189 @@ | |||
| <script lang="ts"> | |||
| import { CommandPalette, Toggle } from '$lib'; | |||
| import { goto } from '$app/navigation'; | |||
There was a problem hiding this comment.
Fix goto() calls to respect the app base.
The goto() calls on lines 43 and 75 are missing resolve(), which will cause navigation to fail when the app is deployed with a non-root config.kit.paths.base. Import resolve from $app/navigation and wrap the paths before calling goto().
Apply this diff:
- import { goto } from '$app/navigation';
+ import { goto, resolve } from '$app/navigation';And update the navigation calls:
- onselect: () => goto('/')
+ onselect: () => {
+ void goto(resolve('/'));
+ }- onselect: () => goto('/docs/pages/introduction')
+ onselect: () => {
+ void goto(resolve('/docs/pages/introduction'));
+ }Also applies to: 43-43, 75-75
🤖 Prompt for AI Agents
In src/routes/docs-examples/extend/command-palette/+page.svelte around line 3
and navigation calls at lines 43 and 75, the goto() calls do not use resolve()
so navigation will break when the app is deployed with a non-root base path;
update the import to: import { goto, resolve } from '$app/navigation' and change
the two goto('/some/path') calls to use goto(resolve('/some/path')) (wrap the
existing path arguments with resolve) so the app base is respected.
2 participants
🔗 Related issue (optional)
Closes #
📑 Description
🔍 PR Type
🚦 PR Status
✅ Checklist
pnpm check && pnpm test:e2eand all tests passapi-checkdirectory if a component API changedmainbranch (not the published npm version)🧪 Screenshots / Demos (optional)
ℹ️ Additional Information
Summary by CodeRabbit
New Features
Documentation
Tests