Shadcn multi-select component in sandbox reactive editor #1575
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request updates various aspects of the project. The dependency version for Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
UI unit Tests12 tests 12 ✅ 0s ⏱️ Results for commit 443e710. ♻️ This comment has been updated with latest results. |
frontend/viewer/src/lib/components/field-editors/multi-select.svelte
Outdated
Show resolved
Hide resolved
frontend/viewer/src/lib/components/field-editors/multi-select.svelte
Outdated
Show resolved
Hide resolved
![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 (6)
frontend/viewer/src/lib/entry-editor/field-data.ts (1)
33-36: Note the deprecation of FieldIds typeThe
FieldIdstype is now properly marked as deprecated with a clear migration path to use the more type-safeFieldIdtype. Consider updating usages ofFieldIdsacross the codebase to useFieldIdin future PRs.frontend/viewer/src/lib/components/field-editors/field-title.svelte (1)
30-37: Consider simplifying the word splitting logicThe current implementation for handling the last word separation is a bit complex and introduces extra processing. Consider using CSS to handle the spacing between the label and help icon instead.
- // kind of crazy, but I don't think Svelte's white-space handling let's us use between the label and help icon - const { lastWord, otherWords } = $derived.by(() => { - const words = label.split(' '); - return { - lastWord: words.pop(), - otherWords: words.join(' '), - }; - })And then update the template section to:
<span class="name" {title}> {label} {#if helpId} <span class="inline-block ml-1"><FieldHelpIcon {helpId} /></span> {/if} </span>frontend/viewer/src/lib/views/view-data.ts (1)
42-49: Consider using a getter for alternateView consistencyWhile
FW_CLASSIC_VIEWworks as implemented, for consistency withFW_LITE_VIEW, consider using a getter for thealternateViewproperty here as well.- alternateView: FW_LITE_VIEW, + get alternateView() { return FW_LITE_VIEW; }frontend/viewer/src/lib/components/field-editors/multi-select.svelte (2)
144-149: Handle cases where no item is highlighted.
The helper functiongetHighlightedValue()can returnundefinedif no item is highlighted. While this is handled gracefully in yourtryToggleHighlightedValue()method, consider logging or providing a user hint if toggling fails, particularly for accessibility or discoverability.
237-239: Fix typographical error in comment.
“deault” should be “default.”- // prevents deault command item selection + // prevents default command item selectionfrontend/viewer/src/lib/sandbox/Sandbox.svelte (1)
89-91: Verify randomness approach for shuffling lists.
Usingreturn Math.random() - 0.5inside a custom comparison function can produce random ordering, though it lacks uniformity and stability. If you need a more robust shuffle, consider implementing Fisher–Yates or a similar algorithm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
frontend/viewer/package.json(1 hunks)frontend/viewer/src/app.postcss(1 hunks)frontend/viewer/src/lib/components/field-editors/field-title.svelte(1 hunks)frontend/viewer/src/lib/components/field-editors/multi-select.svelte(1 hunks)frontend/viewer/src/lib/components/ui/button/button.svelte(1 hunks)frontend/viewer/src/lib/components/ui/checkbox/checkbox.svelte(1 hunks)frontend/viewer/src/lib/components/ui/command/command-input.svelte(2 hunks)frontend/viewer/src/lib/components/ui/drawer/drawer-content.svelte(2 hunks)frontend/viewer/src/lib/components/ui/drawer/drawer.svelte(2 hunks)frontend/viewer/src/lib/entry-editor/FieldTitle.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/field-data.ts(1 hunks)frontend/viewer/src/lib/sandbox/OptionSandbox.svelte(2 hunks)frontend/viewer/src/lib/sandbox/Sandbox.svelte(7 hunks)frontend/viewer/src/lib/views/view-data.ts(2 hunks)frontend/viewer/src/project/ProjectDropdown.svelte(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/viewer/src/project/ProjectDropdown.svelte
🧰 Additional context used
🧬 Code Definitions (1)
frontend/viewer/src/lib/views/view-data.ts (1)
frontend/viewer/src/lib/entry-editor/field-data.ts (1)
FieldIds(36-36)
🔇 Additional comments (27)
frontend/viewer/src/lib/components/ui/checkbox/checkbox.svelte (1)
26-26: Improved icon container sizingChanging from fixed size to
size-fullensures the icon properly fills and centers within its container, creating better visual alignment.frontend/viewer/src/lib/components/ui/command/command-input.svelte (1)
10-10: Enhanced component flexibility with children supportThe addition of the
childrenproperty and its conditional rendering allows the command input component to display additional content, which is essential for the multi-select implementation. The optional chaining ensures graceful handling when no children are provided.Also applies to: 26-26
frontend/viewer/src/lib/components/ui/button/button.svelte (1)
24-24: Added compact icon button variantThe new
xs-iconsize variant provides a smaller button option (8x8 units vs 10x10 for regular icon buttons), which is useful for compact UI elements like clear buttons in the multi-select component. This addition enhances the component library's flexibility while maintaining consistent sizing patterns.frontend/viewer/package.json (1)
51-51: Fixed bits-ui dependency version for stabilityLocking the bits-ui version to exactly 1.3.16 (removing the caret from ^1.3.10) ensures consistent behavior across all environments. This prevents automatic updates to newer minor versions that might introduce breaking changes, particularly important for the UI components being modified in this PR.
frontend/viewer/src/lib/components/ui/drawer/drawer-content.svelte (2)
10-10: Good enhancement - adds flexibility to the drawer componentAdding an optional
handleprop with a default value oftruemaintains backward compatibility while allowing customization when needed.Also applies to: 15-15
29-31: Proper implementation of conditional handle renderingThe conditional rendering of the handle element aligns well with the PR objectives, supporting the responsive design approach for the multi-select component.
frontend/viewer/src/lib/entry-editor/FieldTitle.svelte (1)
5-5: Good type safety improvementThe import of the
FieldIdtype and the type casting in the reactive statement enhances type safety by ensuringidis treated as a valid field identifier when accessingfieldData.Also applies to: 11-11
frontend/viewer/src/lib/sandbox/OptionSandbox.svelte (1)
33-34: Proper view type differentiationAdding type properties to distinguish between 'fw-classic' and 'fw-lite' views supports the responsive design approach mentioned in the PR objectives, enabling different component behaviors for desktop and mobile users.
Also applies to: 44-45
frontend/viewer/src/lib/entry-editor/field-data.ts (1)
30-31: Excellent type safety improvementCreating a
FieldIdtype from the keys ofprivateFieldDataand using it to type thefieldDataexport ensures that only valid field identifiers can be used at compile time.frontend/viewer/src/lib/components/field-editors/field-title.svelte (3)
1-5: Clean imports with well-defined dependenciesThe imports are appropriately organized, importing only what's needed for the component's functionality: translation utilities, help icon component, and view service.
6-14: Props with proper type definitionsThe component defines clear prop types using TypeScript, ensuring type safety for the component inputs. The use of
$props()follows Svelte 5 conventions.
40-52: Clear and semantic markup structureThe component uses a clean and semantic structure with appropriate class names. The inline-flex approach ensures proper alignment of the text and icon.
frontend/viewer/src/lib/views/view-data.ts (3)
33-40: Good use of constants and getter for circular reference preventionThe
FW_LITE_VIEWconstant is well-defined with the newtypeproperty, and the use of a getter foralternateViewcleverly prevents circular reference issues that would occur with direct assignments.
55-65: Good reuse of view constants in the views arrayUsing the defined constants in the views array and spreading
FW_LITE_VIEWinto custom views is a clean approach that ensures consistency across view definitions.
91-96: Type safety improvement with union typeAdding the
typeproperty to theViewDefinitioninterface with a specific union type improves type safety and makes view type distinctions explicit in the codebase.frontend/viewer/src/lib/components/ui/drawer/drawer.svelte (4)
2-4: Mobile detection integrationGood addition of the mobile detection hook and necessary imports to enhance drawer behavior across different devices.
14-20: Reactive drawer state handlingThe use of the
watchfunction to react to open state changes is a clean implementation. This ensures proper handling of browser history when the drawer opens or closes.
22-36: Browser history management for mobile UXThe implementation intelligently handles browser history only on mobile devices, ensuring back button behavior works as expected without affecting desktop navigation.
38-52: Proper cleanup with AbortController and lifecycle hooksThe use of
AbortControllerwithonMountandonDestroyensures proper cleanup of event listeners and history state, preventing memory leaks and unexpected navigation behavior.frontend/viewer/src/app.postcss (3)
94-96: New lg-form grid template introductionThe new
.lg-formclass provides a wider layout option with more space for field labels, enhancing the responsive design system.
98-103: Enhanced responsive grid layoutThe updated
.editor-gridclass now applies different grid layouts at different breakpoints, which aligns with the PR's focus on responsive design for both desktop and mobile users.
105-121: Well-structured field component CSS classesThe new field-related classes (
.field-root,.field-title,.field-body) create a consistent and responsive layout system for field components. The use of subgrid ensures proper alignment across nested grids.frontend/viewer/src/lib/components/field-editors/multi-select.svelte (3)
18-34: Confirm$bindable()usage for Svelte compatibility.
It appears that these lines rely on non-standard Svelte reactivity helpers, particularly$bindable(). Please verify that integrating this feature works correctly in your build environment and does not conflict with standard Svelte patterns or any bundling plugins.
254-286: Mobile and desktop integration looks solid.
Your Drawer-based approach for mobile users, alongside the Popover for desktop, is a well-structured solution. The distinct flows for “Dismiss” vs. “Submit” align with the design goals, and the toggled states (dirty, open) appear to be managed cleanly.
102-122:✅ Verification successful
Consider avoiding in-place array manipulation for reactive updates.
CallingpendingValues.splice(index, 1);mutates the array in-place. While it may work with$stateor certain store libraries, in many reactive contexts, splicing can sometimes lead to stale reactivity. To ensure consistent behavior, consider creating a new array instead:- if (index !== -1) pendingValues.splice(index, 1); + if (index !== -1) { + pendingValues = [ + ...pendingValues.slice(0, index), + ...pendingValues.slice(index + 1) + ]; + }
Action: Update reactive removal in
multi-select.svelteto avoid in-place mutationIn the
toggleSelectedfunction, while adding values a new array is created to trigger reactivity, the removal branch uses in-place mutation withsplice, which may not update the UI as expected. Instead, reassign a new array after removal to ensure consistent reactive updates. For example, replace:- if (index !== -1) pendingValues.splice(index, 1); + if (index !== -1) { + pendingValues = [ + ...pendingValues.slice(0, index), + ...pendingValues.slice(index + 1) + ]; + }This change aligns with Svelte’s reactive principles and avoids potential stale reactivity issues.
frontend/viewer/src/lib/sandbox/Sandbox.svelte (2)
155-156: Confirm two-way binding approach forvalues.
Your usage ofbind:values={() => selectedDomains, (newValues) => selectedDomains = newValues}is unconventional compared to standard Svelte’sbind:values={variable}. Verify that this syntax works as expected with your chosen reactivity library or store.
2-32: Imports and layout adjustments look good.
These additions integrate new UI components (e.g.,FieldTitle,ThemePicker,Tabs,Switch) and reorganize the layout. No major issues are apparent, and the approach seems consistent with the rest of the codebase.Also applies to: 25-31, 102-104, 115-117, 127-139, 142-142, 146-149, 152-166, 202-202, 206-206, 282-289
|
Looks great! One issue I noticed in dark mode there's a bg-popover on the command which in dark mode looks like this on the mobile view: notice the grey background only under the command area. I think it would look fine without that background: or if there was no rounding on the corners: but right now it feels cramped with the background having rounded corners. Otherwise it looks great! Nice work |
|
Regarding the weird background color: |
Resolves #1580
localhost_5137_sandbox.-.Google.Chrome.2025-04-07.16-44-50.mp4
Updates:
Badges/header in mobile drawer so the user has context:
keyboard navigation
A sort strategy can be specified for the selected values
Moved the submit button on desktop to inside the filter input, so there isn't a layout shift and it's nice and visible.
Readonly mode
