Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Export TimelineConfig and TimelineConfigSchema from @object-ui/types (#64) - Export NavigationConfig and NavigationConfigSchema from @object-ui/types - Update ObjectTimeline to use spec-compliant startDateField with backward compat (#72) - Add endDateField, groupByField, colorField, scale to ObjectTimeline - Add navigation support to ObjectGallery via useNavigationOverlay (#66) - Implement navigation.view property in useNavigationOverlay hook (#68) - Implement emptyState spec property in ListView (#71) - Add emptyState to ListViewSchema type - Add tests for all new functionality Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…chema fix Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR focuses on Priority 0 spec-compliance work across view plugins and types, aligning ObjectUI’s runtime behavior and type exports with @objectstack/spec (timeline config, list-view empty state, and navigation view targeting).
Changes:
- Added
emptyStatetoListViewSchema(types) and rendered an empty state UI inListViewwhen there’s no data. - Implemented
navigation.viewsupport inuseNavigationOverlay(propagated toonNavigateandnew_windowURLs) with accompanying tests. - Extended timeline config handling in
ObjectTimelineto supportstartDateField(withdateFieldbackward compat) plusendDateField/groupByField/colorField/scale, and added ObjectGallery navigation overlay support.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/objectql.ts | Adds emptyState typing to ListViewSchema for spec alignment. |
| packages/types/src/index.ts | Re-exports TimelineConfig* and NavigationConfig* from the spec package. |
| packages/react/src/hooks/useNavigationOverlay.ts | Adds view handling and includes it in navigation behavior + returned hook state. |
| packages/react/src/tests/useNavigationOverlay.test.ts | Adds coverage for navigation.view behavior (page + new_window). |
| packages/plugin-timeline/src/ObjectTimeline.tsx | Adds spec-compliant timeline field options and backward compatibility mapping. |
| packages/plugin-timeline/src/ObjectTimeline.test.tsx | Adds tests for startDateField, grouping/color fields, and scale. |
| packages/plugin-list/src/ObjectGallery.tsx | Integrates useNavigationOverlay + NavigationOverlay into gallery cards. |
| packages/plugin-list/src/tests/ObjectGallery.test.tsx | Introduces initial ObjectGallery navigation tests. |
| packages/plugin-list/src/ListView.tsx | Renders empty state when data.length === 0 and updates timeline option wiring for startDateField. |
| packages/plugin-list/src/tests/ListView.test.tsx | Adds tests for default/custom empty state rendering. |
| packages/components/src/custom/navigation-overlay.tsx | Extends overlay props type to include view. |
| className={cn( | ||
| 'group overflow-hidden transition-all hover:shadow-md', | ||
| (props.onCardClick || props.onRowClick || schema.navigation) && 'cursor-pointer', | ||
| )} | ||
| onClick={() => navigation.handleClick(item)} |
There was a problem hiding this comment.
The cursor-pointer styling is enabled whenever schema.navigation is present, even if navigation is effectively disabled (mode: 'none' or preventNavigation: true). This can make cards look clickable while clicks do nothing. Gate the pointer styling on the effective navigation behavior (or on navigation.mode/navigation.preventNavigation).
| it('renders with cursor-pointer when navigation is configured', () => { | ||
| const schema = { | ||
| objectName: 'products', | ||
| navigation: { mode: 'drawer' as const }, | ||
| }; | ||
| render(<ObjectGallery schema={schema} data={mockItems} />); | ||
|
|
||
| const cards = screen.getAllByTestId('gallery-card'); | ||
| expect(cards.length).toBe(2); | ||
| }); |
There was a problem hiding this comment.
This test name says it validates the cursor-pointer behavior, but it only asserts the number of rendered cards. Assert on the rendered className (or on a user-visible behavior that depends on the cursor styling) so the test actually covers what it claims.
| it('renders with cursor-pointer when onCardClick is provided', () => { | ||
| const onCardClick = vi.fn(); | ||
| const schema = { objectName: 'products' }; | ||
| render(<ObjectGallery schema={schema} data={mockItems} onCardClick={onCardClick} />); | ||
|
|
||
| const cards = screen.getAllByTestId('gallery-card'); | ||
| fireEvent.click(cards[0]); | ||
|
|
||
| expect(mockHandleClick).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This test creates an onCardClick spy but never asserts it was called; instead it asserts mockHandleClick was called (which comes from the mocked hook). Update the assertion to verify onCardClick is invoked when provided (and consider resetting mockHandleClick between tests to avoid cross-test contamination).
| if (!navigation) { | ||
| const recordId = record._id || record.id; | ||
| if (onNavigate && recordId != null) { | ||
| onNavigate(recordId as string | number, 'view'); | ||
| onNavigate(recordId as string | number, view ?? 'view'); | ||
| } |
There was a problem hiding this comment.
handleClick uses the view value when calling onNavigate, but view is not included in the useCallback dependency list. If the navigation config is mutated in-place (or view changes without a new navigation object), the click handler can keep using a stale view. Add view to the dependency array (or read navigation?.view inside the callback).
| const recordId = record._id || record.id; | ||
| const url = objectName ? `/${objectName}/${recordId}` : `/${recordId}`; | ||
| const viewPath = view ? `/${view}` : ''; | ||
| const url = objectName ? `/${objectName}/${recordId}${viewPath}` : `/${recordId}${viewPath}`; | ||
| window.open(url, '_blank'); | ||
| return; |
There was a problem hiding this comment.
For new_window mode, window.open(url, '_blank') should include noopener,noreferrer to prevent reverse-tabnabbing, and it should guard against missing recordId (currently could open .../undefined). Also consider URL-encoding recordId/view segments before building the path.
| const iconName = schema.emptyState?.icon; | ||
| const ResolvedIcon: LucideIcon = iconName | ||
| ? ((icons as Record<string, LucideIcon>)[ | ||
| iconName.split('-').map(w => w.charAt(0).toUpperCase() + w.slice(1)).join('') | ||
| ] ?? Inbox) | ||
| : Inbox; |
There was a problem hiding this comment.
schema.emptyState?.icon is used as a string without a runtime type check (iconName.split(...)). If a non-string value is provided, this will throw at render time. Also, this local kebab→Pascal conversion doesn’t apply the existing Lucide rename mapping used elsewhere (e.g. Home→House), so some valid icon names may fail to resolve; consider centralizing this logic or reusing the existing resolver.
| const iconName = schema.emptyState?.icon; | |
| const ResolvedIcon: LucideIcon = iconName | |
| ? ((icons as Record<string, LucideIcon>)[ | |
| iconName.split('-').map(w => w.charAt(0).toUpperCase() + w.slice(1)).join('') | |
| ] ?? Inbox) | |
| : Inbox; | |
| const rawIcon = schema.emptyState?.icon; | |
| let ResolvedIcon: LucideIcon = Inbox; | |
| if (typeof rawIcon === 'string' && rawIcon.trim().length > 0) { | |
| const iconMap = icons as Record<string, LucideIcon>; | |
| // Try direct key match first (e.g. "Inbox", "home", "House") | |
| const directMatch = iconMap[rawIcon] ?? iconMap[rawIcon.charAt(0).toUpperCase() + rawIcon.slice(1)]; | |
| if (directMatch) { | |
| ResolvedIcon = directMatch; | |
| } else { | |
| const pascalName = rawIcon | |
| .split('-') | |
| .map(w => (w ? w.charAt(0).toUpperCase() + w.slice(1) : '')) | |
| .join(''); | |
| ResolvedIcon = iconMap[pascalName] ?? Inbox; | |
| } | |
| } |
|
|
||
| // Check for Timeline capabilities | ||
| if (schema.options?.timeline?.dateField || schema.options?.calendar?.startDateField) { | ||
| if (schema.options?.timeline?.startDateField || schema.options?.timeline?.dateField || schema.options?.calendar?.startDateField) { |
There was a problem hiding this comment.
Timeline view availability is currently enabled when schema.options.calendar.startDateField is present. This will show the Timeline view even when no timeline config exists. The Timeline capability check should depend only on schema.options.timeline fields.
| if (schema.options?.timeline?.startDateField || schema.options?.timeline?.dateField || schema.options?.calendar?.startDateField) { | |
| if (schema.options?.timeline?.startDateField || schema.options?.timeline?.dateField) { |
| // --- NavigationConfig support --- | ||
| const navigation = useNavigationOverlay({ | ||
| navigation: schema.navigation, | ||
| objectName: schema.objectName, | ||
| onRowClick: props.onRowClick ?? props.onCardClick, | ||
| }); |
There was a problem hiding this comment.
ObjectGallery wires useNavigationOverlay without providing an onNavigate handler, and schema doesn’t expose one. In page mode (the default), clicks will become a no-op (no overlay, no navigation). Add onNavigate support (in schema/props) and pass it into useNavigationOverlay so page navigation works consistently with other view plugins.
Implements all Priority 0 v1.0 UI Essentials from ROADMAP_SPEC.md (tasks #64, #66, #68, #71, #72).
TimelineConfig spec alignment (#64, #72)
TimelineConfigandTimelineConfigSchemafrom@object-ui/typesdateField→startDateFieldwith backward compat fallbackendDateField,groupByField,colorField,scaleto ObjectTimelineObjectGallery navigation (#66)
useNavigationOverlay+NavigationOverlayinto ObjectGallerynavigationconfig andonRowClickprop, matching all other view pluginsnavigation.viewproperty (#68)viewfromuseNavigationOverlayreturn valueviewtoonNavigate(recordId, view)for page modenew_windowmode:/{object}/{id}/{view}ListView
emptyState(#71)emptyState?: { title?, message?, icon? }toListViewSchemaAlso exported
NavigationConfigandNavigationConfigSchemafrom@object-ui/types(from spec)Already complete (verified)
GalleryConfigalready exportednavigation.widthalready flows via{...navigation}spread in all view pluginsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.