-
Notifications
You must be signed in to change notification settings - Fork 5
Fix PluginObjectView split/popover navigation modes falling back to drawer #891
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
Changes from all commits
d55f2b0
e4d1d50
88716d9
968eb4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -49,6 +49,7 @@ import { | |||||
| DrawerHeader, | ||||||
| DrawerTitle, | ||||||
| DrawerDescription, | ||||||
| NavigationOverlay, | ||||||
| Button, | ||||||
| Tabs, | ||||||
| TabsList, | ||||||
|
|
@@ -974,8 +975,57 @@ export const ObjectView: React.FC<ObjectViewProps> = ({ | |||||
| // Determine which form container to render | ||||||
| const formLayout = navigationConfig?.mode === 'modal' ? 'modal' | ||||||
| : navigationConfig?.mode === 'drawer' ? 'drawer' | ||||||
| : navigationConfig?.mode === 'split' ? 'split' | ||||||
| : navigationConfig?.mode === 'popover' ? 'popover' | ||||||
| : layout; | ||||||
|
|
||||||
| // Build the record detail content for NavigationOverlay (split/popover modes) | ||||||
| const renderOverlayDetail = (record: Record<string, unknown>) => ( | ||||||
|
||||||
| const renderOverlayDetail = (record: Record<string, unknown>) => ( | |
| const renderOverlayDetail = (_record: Record<string, unknown>) => ( |
Copilot
AI
Feb 27, 2026
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 split-mode branch duplicates the outer layout structure (title/description/toolbar wrappers) from the main return path. This increases the risk of future UI divergence between split and non-split modes. Consider extracting a shared wrapper/layout component/function so split mode only changes the content region wiring.
Copilot
AI
Feb 27, 2026
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.
In split navigation mode, the overlay is only rendered when isFormOpen && selectedRecord. This means create-mode (where selectedRecord is intentionally null) will not render any form at all, and edit/view behavior becomes dependent on selectedRecord being set. Consider using split/popup overlays only for record-detail view (e.g., formMode === 'view'), or falling back to the existing drawer/modal form container for create/edit so those flows keep working when navigation.mode is split.
Copilot
AI
Feb 27, 2026
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.
Popover mode has the same isFormOpen && selectedRecord gate. As a result, create-mode (no selectedRecord) cannot open a form when navigation.mode is popover because neither drawer/modal nor NavigationOverlay will render. Recommend ensuring create/edit continue to render via the normal form container, and reserving the popover NavigationOverlay for record-detail view only.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -300,8 +300,9 @@ describe('ObjectView', () => { | |
|
|
||
| fireEvent.click(screen.getByTestId('grid-row')); | ||
|
|
||
| // Split mode should trigger form display in view mode | ||
| // Split mode renders NavigationOverlay with split panels including a close button | ||
| expect(screen.getByTestId('object-form')).toBeDefined(); | ||
| expect(screen.getByLabelText('Close panel')).toBeDefined(); | ||
| }); | ||
|
Comment on lines
+303
to
306
|
||
|
|
||
| it('should open form in view mode when popover navigation mode is clicked', () => { | ||
|
|
@@ -315,8 +316,30 @@ describe('ObjectView', () => { | |
|
|
||
| fireEvent.click(screen.getByTestId('grid-row')); | ||
|
|
||
| // Popover mode should trigger form display in view mode | ||
| // Popover mode renders NavigationOverlay Dialog fallback (no popoverTrigger) | ||
| expect(screen.getByTestId('object-form')).toBeDefined(); | ||
| expect(screen.getByRole('dialog')).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should close split panel and return to normal view', () => { | ||
| const schema: ObjectViewSchema = { | ||
| type: 'object-view', | ||
| objectName: 'contacts', | ||
| navigation: { mode: 'split' }, | ||
| }; | ||
|
|
||
| render(<ObjectView schema={schema} dataSource={mockDataSource} />); | ||
|
|
||
| // Open split panel | ||
| fireEvent.click(screen.getByTestId('grid-row')); | ||
| expect(screen.getByLabelText('Close panel')).toBeDefined(); | ||
|
|
||
| // Close split panel | ||
| fireEvent.click(screen.getByLabelText('Close panel')); | ||
|
|
||
| // Form should be gone, grid should remain | ||
| expect(screen.queryByLabelText('Close panel')).toBeNull(); | ||
| expect(screen.getByTestId('object-grid')).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
|
|
||
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.
useNavigationOverlaycallsonNavigate(recordId, view ?? 'view')for page mode, whereviewcomes fromnavigation.view(a user-configurable target view name). With the new guard, any configurednavigation.viewvalue other than'view'will prevent navigation entirely. Consider treating any action other than'new_window'as page navigation (or explicitly handlingnavigation.viewby incorporating it into the route/query) so page mode keeps working when a view name is set.