Shadcn low-hanging fruit follow-up#1693
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 update introduces a new Changes
Poem
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 (
|
UI unit Tests12 tests 12 ✅ 0s ⏱️ Results for commit 8f5e9b0. ♻️ This comment has been updated with latest results. |
![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 (4)
frontend/viewer/src/lib/components/reorderer/reorderer-item-list.svelte (1)
46-46: Confirm removal of pointer cursor for reorder items.
Droppingcursor-pointerchanges the hover feedback on draggable items. If intentional, ensure this aligns with the UX guidelines. Alternatively, consider using acursor-graborcursor-moveto indicate draggable functionality.frontend/viewer/src/project/browse/EntryMenu.svelte (1)
66-66: Simplify conditional logicThe current condition uses
|| truewhich effectively makes the condition only dependent onfeatures.openWithFlex. This approach adds unnecessary complexity.- {#if features.openWithFlex && (appLauncher || !IsMobile.value || true)} + {#if features.openWithFlex}frontend/viewer/src/lib/utils/tabbable.ts (1)
1-17: Well-implemented focus utility functionThis utility function is a clean implementation for finding the first tabbable element in a container. It properly handles null/undefined containers and uses the efficient TreeWalker API for DOM traversal.
Consider adding JSDoc comments to document the purpose and usage of this function, especially since it's likely to be used across multiple components.
+/** + * Finds the first tabbable element within a container. + * @param container - The container element to search within + * @returns The first tabbable HTMLElement or undefined if none found + */ export function findFirstTabbable(container: Element | null | undefined): HTMLElement | undefined {frontend/viewer/src/lib/components/ui/icon/icon.svelte (1)
25-25: Consider explicitly extracting the size prop.The
sizevariant is defined iniconVariantsbut not explicitly extracted from props.- const { class: className, ...restProps }: IconProps = $props(); + const { class: className, size = "default", ...restProps }: IconProps = $props();Then use it when applying variants:
- <img {...restProps} class={cn(iconVariants(), className)} /> + <img {...restProps} class={cn(iconVariants({ size }), className)} />(And similar for the span element)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
frontend/viewer/package.json(1 hunks)frontend/viewer/src/home/HomeView.svelte(3 hunks)frontend/viewer/src/home/Server.svelte(1 hunks)frontend/viewer/src/lib/OpenInFieldWorksButton.svelte(2 hunks)frontend/viewer/src/lib/components/editor/editor-root.svelte(1 hunks)frontend/viewer/src/lib/components/reorderer/reorderer-item-list.svelte(1 hunks)frontend/viewer/src/lib/components/responsive-menu/responsive-menu-item.svelte(2 hunks)frontend/viewer/src/lib/components/ui/button/index.ts(1 hunks)frontend/viewer/src/lib/components/ui/context-menu/context-menu-item.svelte(1 hunks)frontend/viewer/src/lib/components/ui/dropdown-menu/dropdown-menu-item.svelte(1 hunks)frontend/viewer/src/lib/components/ui/icon/icon.svelte(1 hunks)frontend/viewer/src/lib/components/ui/icon/index.ts(1 hunks)frontend/viewer/src/lib/components/ui/scroll-area/scroll-area.svelte(2 hunks)frontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/ItemListItem.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.svelte(9 hunks)frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte(1 hunks)frontend/viewer/src/lib/utils/tabbable.ts(1 hunks)frontend/viewer/src/project/ProjectDropdown.svelte(3 hunks)frontend/viewer/src/project/browse/BrowseView.svelte(2 hunks)frontend/viewer/src/project/browse/EntriesList.svelte(1 hunks)frontend/viewer/src/project/browse/EntryMenu.svelte(1 hunks)frontend/viewer/src/project/browse/EntryView.svelte(5 hunks)frontend/viewer/src/project/browse/SearchFilter.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build API / publish-api
- GitHub Check: Build UI / publish-ui
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (43)
frontend/viewer/package.json (1)
100-100: Appropriate dependency addition for focus managementAdding the "tabbable" library aligns with the PR objective of improving keyboard navigation and enhancing tabbing behavior. This library helps identify and manage focus on interactive elements.
frontend/viewer/src/lib/components/ui/dropdown-menu/dropdown-menu-item.svelte (1)
18-18: Improved usability with cursor-pointerChanging from
cursor-defaulttocursor-pointerprovides better visual feedback to users that the dropdown items are interactive. This is consistent with standard UI patterns and aligns with the visual improvements mentioned in the PR objectives.frontend/viewer/src/lib/components/editor/editor-root.svelte (2)
9-14: Appropriate change from const to let for DOM bindingThe change from
consttoletfor the destructured props is necessary to support the DOM element binding added in line 17. This is standard practice in Svelte when variables need to be reassigned.
17-17: Improved accessibility with ref bindingBinding the ref to the root div element enables parent components to access this DOM element directly, supporting the PR objectives of improved focus management and scrolling behavior.
frontend/viewer/src/lib/components/ui/scroll-area/scroll-area.svelte (2)
9-9: Well-implemented viewport reference propertyAdding the viewportRef bindable property with proper typing enables external components to access the viewport DOM element, supporting the scrolling and focus management improvements mentioned in the PR objectives.
Also applies to: 21-21
28-28: Proper DOM binding implementationThe binding between the viewportRef prop and the ScrollAreaPrimitive.Viewport element is correctly implemented, allowing parent components to control scrolling behavior programmatically.
frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte (1)
38-38: Verify presence ofeditor-primitiveCSS definition.
You added theeditor-primitiveclass to the editor grid container to unify styling and focus behavior. Please ensure this class is defined in your global or component stylesheet (e.g., in a shared CSS module or Tailwind config) so that it has the intended effect.frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (1)
32-32: Consistent addition ofeditor-primitiveclass.
This aligns the example editor primitive with others, enabling uniform styling and focus behavior. Ensure that your focus management logic (e.g.,findFirstTabbable) targets this class appropriately.frontend/viewer/src/lib/entry-editor/ItemListItem.svelte (1)
71-71: Enhanced click affordance by adding pointer cursor.
Thecursor-pointerclass clearly indicates interactivity on the Remove item, improving usability and consistency with other menu items. Great enhancement!frontend/viewer/src/lib/components/ui/context-menu/context-menu-item.svelte (1)
18-18: Switch to pointer cursor for better affordance.
Replacingcursor-defaultwithcursor-pointerimproves the visual feedback on hover, signaling that the item is clickable. This aligns with similar updates in dropdown menus.frontend/viewer/src/lib/OpenInFieldWorksButton.svelte (2)
11-11: Good import addition for icon standardization.Adding the Icon component import aligns with the PR objective of unifying icons across the application.
56-56: Improved icon rendering approach.Replacing direct image usage with the Icon component ensures consistent sizing and alignment for the FieldWorks logo, as mentioned in the PR objectives.
frontend/viewer/src/project/browse/EntriesList.svelte (1)
70-72: Good optimization with debounce.Adding a 300ms debounce to the resource call prevents excessive API requests when search parameters change rapidly, improving performance and user experience.
frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (1)
36-36: Good addition for focus management.Adding the
editor-primitiveclass standardizes styling across editor components and enables improved focus management with the new tabbable utility, supporting the PR's goal of enhancing keyboard navigation.frontend/viewer/src/project/browse/BrowseView.svelte (2)
7-7: Good refactoring of badge import.Switching from the Badge component to badgeVariants function import follows a more flexible styling utility approach.
59-65: Improved button implementation.Replacing the Badge component with a native button element styled using badgeVariants provides better semantics while maintaining consistent styling. This approach is more flexible and aligns with modern UI development patterns.
frontend/viewer/src/lib/components/ui/icon/index.ts (1)
1-10: Improved component exports with proper type informationThe Icon component is now properly exported along with its type definitions and variants, following best practices for component modularity and reuse.
frontend/viewer/src/home/HomeView.svelte (3)
32-32: LGTM: Icon component import addedThe Icon component is now imported to be used for standardizing icon rendering throughout the file.
108-108: Standardized logo renderingReplacing the direct
<img>with the Icon component improves consistency across the UI.
226-226: Consistent FieldWorks logo implementationThe FieldWorks logo is now also using the Icon component, ensuring consistent rendering and styling.
frontend/viewer/src/home/Server.svelte (1)
126-132: Improved UI structure for helper buttonMoving the "I don't see my project" button outside the project list container creates a better visual hierarchy and ensures the button appears only once regardless of the number of projects.
frontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.svelte (1)
22-28: Improved menu item interactivity and compositionThe addition of
cursor-pointerclass makes the dropdown item's clickability more evident to users. The use of the{#snippet child({props})}pattern enables better component composition by allowing props to be passed down to the Link component.frontend/viewer/src/project/browse/SearchFilter.svelte (1)
10-10: Enhanced component composition with prop mergingThe addition of
mergePropsand the{#snippet child({props})}pattern creates a more composable UI by allowing props to be passed down and merged with existing ones. This approach ensures the Toggle maintains its required properties while accepting additional ones from its parent.Also applies to: 51-55
frontend/viewer/src/project/browse/EntryView.svelte (4)
5-5: Added necessary imports for enhanced user experience featuresThe new imports support the added functionality for improved focus management and viewport scrolling.
Also applies to: 20-21
45-45: Properly handled promise with void operatorGood practice to use the
voidoperator to explicitly ignore the promise returned byrefetch().
57-63: Enhanced user experience with automatic scrolling and focusThis watch effect is well-implemented to improve the user experience when navigating between entries by:
- Automatically scrolling to the top of the entry
- Setting focus on the first interactive element (skipping this on mobile devices)
The conditional check for mobile is a good UX consideration as automatic focus can be disruptive on touch devices.
98-98: Proper binding of DOM referencesThe component references are properly bound to enable the scrolling and focus functionality implemented in the watch effect.
Also applies to: 103-103
frontend/viewer/src/project/ProjectDropdown.svelte (3)
65-71: Well-organized icon rendering logic!Creating a reusable snippet for the project icon rendering is a great DRY improvement that centralizes the conditional logic in one place.
84-84: Clean implementation of the icon snippetReplacing the duplicated icon rendering logic with the snippet calls makes the code more maintainable and consistent.
Also applies to: 115-115
108-108: Proper placement of Command.EmptyMoving the Command.Empty inside the else block ensures it only appears after loading completes and when no projects are found, which is the correct behavior.
frontend/viewer/src/lib/components/ui/button/index.ts (1)
10-20: Improved export organizationThe reorganized exports with additional aliases provide more flexible and consistent API usage patterns across the UI components.
frontend/viewer/src/lib/components/responsive-menu/responsive-menu-item.svelte (4)
7-13: Well-structured importsThe reorganized imports properly separate direct component imports from type imports, which improves code organization.
20-20: Improved type definitionThe Props type now correctly combines ContextMenuItemProps and DrawerCloseProps while avoiding conflicts with the onclick handler.
48-49: Consistent event bindingDirect binding of the onSelect handler is more consistent with the component's API design.
Also applies to: 53-54
58-64: Semantic improvement with DrawerCloseReplacing Button with DrawerClose for the mobile view is semantically more appropriate and provides better integration with the drawer component.
frontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.svelte (5)
10-10: Good component API enhancementAdding the ref prop and binding it to the root element allows external components to access and manipulate the editor's DOM element when needed.
Also applies to: 35-35, 156-156
31-31: Improved focus management setupThe findFirstTabbable utility import and enhanced highlightedEntity state structure provide better support for focus management.
Also applies to: 113-113
53-53: Good distinction between highlight and focus behaviorsThe code now properly distinguishes between operations that should trigger autofocus (adding new items) and those that should only highlight (moving items).
Also applies to: 60-60, 80-80, 99-99
124-126: Well-implemented autofocus logicThe conditional focus handling respects mobile devices (where focus can be problematic) and uses the findFirstTabbable utility to locate the appropriate element to focus.
161-161: Updated class bindingsThe class bindings have been correctly updated to work with the new highlightedEntity structure.
Also applies to: 179-179
frontend/viewer/src/lib/components/ui/icon/icon.svelte (3)
8-18: Good implementation of tailwind-variants for consistent styling.The use of
tailwind-variantsfor centralizing icon styling is a good approach. The base styles and size variants are well-defined, making it easy to maintain consistent styling across different usages of the component.
20-21: Strong type definition for polymorphic component.The union type approach for
IconPropsis a good solution to support both icon classes and image sources in a type-safe manner. This ensures proper type checking for both usage patterns.
30-34: Good technique for baseline alignment.The non-breaking space in the span element is a clever way to ensure consistent baseline alignment. The descriptive comment is also helpful for future maintainers.
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
looks good, I left a few things to discuss
|
we're fixing nitpicks so here's one: |
2 participants
The most visual changes:
"I don't see my project" accidently got inside the bordered list if projects. I moved it out:
Scroll to top and focus first field:
https://github.com/user-attachments/assets/34c5520a-66b0-4f92-8ce2-3d4e5d754a8e
Tabbing:
https://github.com/user-attachments/assets/dab3ce70-0bd7-4d67-8786-cdb4fb1a0d6e
FieldWorks and Lexbox icons now share the Icon component (i.e. sizing), so alignment should always just work:
Autofocusing first field of new senses and examples:
localhost_5137_fwdata_rmunn-thai-food-test_browse_entryId.346b4126-a24a-4bf5-91eb-a567a647f6db.-.Google.Chrome.2025-05-21.16-20-52.mp4