Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…debar toggle 1. AppShell: Hide SidebarTrigger on mobile (md:hidden) to avoid duplicate with AppHeader's trigger 2. action-bar: Make maxVisible responsive - default 1 on mobile, 3 on desktop 3. DetailView: Hide share/edit/inline-edit buttons on mobile, add them to More menu 4. DetailSection: Use 2-column grid on mobile for better space utilization Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ove JSDoc Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 应该根据不同的宽度决定有多少列呀，怎么能写死。这是一个元数据驱动的平台，你要从长远考虑考虑主流业务系统的做法来评估该怎么开发。 |
…ad of hardcoding Use sm:grid-cols-2 (640px) instead of md:grid-cols-2 (768px) so larger phones get 2 columns, while narrow screens still fall back to 1 column. Progressive: 1 col → sm:2 cols → lg:3 cols. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Fixed in
The threshold is lowered from |
There was a problem hiding this comment.
Pull request overview
Improves the mobile detail-page experience across the layout shell, action toolbars, and detail views by removing duplicated sidebar toggles, reducing action button clutter, and making detail fields use screen width more effectively.
Changes:
- Hide AppShell’s sidebar trigger on smaller viewports to avoid duplicating the AppHeader mobile trigger.
- Make
action:baradapt its inline action count on mobile viauseIsMobile()and a newmobileMaxVisibleschema prop. - Adjust DetailView header actions and DetailSection grid breakpoints for a cleaner mobile layout.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/plugin-detail/src/DetailView.tsx | Hides some header actions on small screens and mirrors them into the “More Actions” dropdown. |
| packages/plugin-detail/src/DetailSection.tsx | Updates responsive grid column breakpoints (md→sm) to use width more effectively on smaller devices. |
| packages/layout/src/AppShell.tsx | Hides AppShell’s built-in sidebar trigger on mobile to prevent duplicate toggles with AppHeader. |
| packages/components/src/renderers/action/action-bar.tsx | Adds mobile-aware maxVisible logic and introduces mobileMaxVisible schema support. |
Comments suppressed due to low confidence (2)
packages/components/src/renderers/action/action-bar.tsx:50
mobileMaxVisibleis added to the schema interface, but it isn't exposed in the ComponentRegistry config (inputs/defaultProps). That means schema-driven tooling/designers can't set it and the default isn't documented alongsidemaxVisible. Add an input entry (and optionally a default prop) formobileMaxVisibleto keep the registry metadata in sync with the renderer behavior.
/** Maximum visible inline actions before overflow into "More" menu (default: 3) */
maxVisible?: number;
/** Maximum visible inline actions on mobile devices (default: 1). Desktop uses maxVisible instead. */
mobileMaxVisible?: number;
packages/plugin-detail/src/DetailView.tsx:554
- These changes introduce new mobile-only behavior (Share/Edit/Inline Edit moved into the More Actions dropdown and hidden inline). There are existing
DetailViewtests, but none assert the new mobile menu items exist and trigger the correct handlers when the inline buttons are hidden. Add/adjust tests to cover the dropdown items (and thesm:hidden/sm:inline-flexsplit) to prevent regressions.
{/* Inline Edit Toggle - hidden on mobile, accessible via more menu */}
{inlineEdit && (
<Tooltip>
<TooltipTrigger asChild>
<Button
variant={isInlineEditing ? 'default' : 'outline'}
size="sm"
onClick={handleInlineEditToggle}
className="gap-2 hidden sm:inline-flex"
>
{isInlineEditing ? (
<>
<Check className="h-4 w-4" />
<span className="hidden sm:inline">{t('detail.save')}</span>
</>
) : (
<>
<Edit className="h-4 w-4" />
<span className="hidden sm:inline">{t('detail.editInline')}</span>
</>
)}
</Button>
</TooltipTrigger>
<TooltipContent>
{isInlineEditing ? t('detail.saveChanges') : t('detail.editFieldsInline')}
</TooltipContent>
</Tooltip>
)}
{/* Share Button - hidden on mobile, accessible via more menu */}
<Tooltip>
<TooltipTrigger asChild>
<Button variant="outline" size="icon" onClick={handleShare} className="hidden sm:inline-flex">
<Share2 className="h-4 w-4" />
</Button>
</TooltipTrigger>
<TooltipContent>{t('detail.share')}</TooltipContent>
</Tooltip>
{/* Edit Button - hidden on mobile, accessible via more menu */}
{schema.showEdit && (
<Tooltip>
<TooltipTrigger asChild>
<Button variant="default" onClick={handleEdit} className="gap-2 hidden sm:inline-flex">
<Edit className="h-4 w-4" />
<span className="hidden sm:inline">{t('detail.edit')}</span>
</Button>
</TooltipTrigger>
<TooltipContent>{t('detail.editRecord')}</TooltipContent>
</Tooltip>
)}
{/* More Actions Menu */}
<DropdownMenu>
<Tooltip>
<TooltipTrigger asChild>
<DropdownMenuTrigger asChild>
<Button variant="ghost" size="icon">
<MoreHorizontal className="h-4 w-4" />
</Button>
</DropdownMenuTrigger>
</TooltipTrigger>
<TooltipContent>{t('detail.moreActions')}</TooltipContent>
</Tooltip>
<DropdownMenuContent align="end" className="w-[calc(100vw-2rem)] sm:w-48 max-h-[60vh] overflow-y-auto">
{/* Mobile-only: Share, Edit, Inline Edit */}
<DropdownMenuItem onClick={handleShare} className="sm:hidden">
<Share2 className="h-4 w-4 mr-2" />
{t('detail.share')}
</DropdownMenuItem>
{schema.showEdit && (
<DropdownMenuItem onClick={handleEdit} className="sm:hidden">
<Edit className="h-4 w-4 mr-2" />
{t('detail.edit')}
</DropdownMenuItem>
)}
{inlineEdit && (
<DropdownMenuItem onClick={handleInlineEditToggle} className="sm:hidden">
<Edit className="h-4 w-4 mr-2" />
{isInlineEditing ? t('detail.save') : t('detail.editInline')}
</DropdownMenuItem>
)}
<DropdownMenuSeparator className="sm:hidden" />
| <SidebarTrigger className="-ml-1 hidden md:inline-flex" /> | ||
| <div className="w-px h-4 bg-border mx-1 sm:mx-2 hidden md:block" /> |
There was a problem hiding this comment.
Hiding the only built-in below md makes the sidebar impossible to open on mobile for AppShell consumers that do not render their own mobile trigger in navbar (e.g. AppSchemaRenderer when navbar is undefined, and AppShell Storybook stories). Consider gating this behind an explicit AppShell prop (e.g. hideTriggerOnMobile) or keeping the trigger visible by default and letting apps opt into hiding it when they provide their own header trigger.
| <SidebarTrigger className="-ml-1 hidden md:inline-flex" /> | |
| <div className="w-px h-4 bg-border mx-1 sm:mx-2 hidden md:block" /> | |
| <SidebarTrigger className="-ml-1 inline-flex" /> | |
| <div className="w-px h-4 bg-border mx-1 sm:mx-2 block" /> |
| @@ -82,7 +86,10 @@ const ActionBarRenderer = forwardRef<HTMLDivElement, { schema: ActionBarSchema; | |||
| }, [schema.actions, schema.location]); | |||
|
|
|||
| // Split into visible inline actions and overflow | |||
| const maxVisible = schema.maxVisible ?? 3; | |||
| // On mobile, show fewer actions inline (default: 1) | |||
| const maxVisible = isMobile | |||
| ? (schema.mobileMaxVisible ?? 1) | |||
| : (schema.maxVisible ?? 3); | |||
There was a problem hiding this comment.
useIsMobile() relies on window.matchMedia() (and addEventListener) without a guard. Now that ActionBar always calls it, environments that don't provide matchMedia (unit tests, some non-browser runtimes) will throw during effect execution. Either make useIsMobile resilient when window.matchMedia is missing, or provide a global matchMedia mock in the test setup so ActionBar rendering can't crash.
| effectiveColumns === 2 ? "grid-cols-1 sm:grid-cols-2" : | ||
| effectiveColumns === 3 ? "grid-cols-1 sm:grid-cols-2 lg:grid-cols-3" : | ||
| "grid-cols-1 sm:grid-cols-2 lg:grid-cols-3" |
There was a problem hiding this comment.
This breakpoint change (md→sm) will break existing unit tests that assert md:grid-cols-2 / md:grid-cols-2 lg:grid-cols-3 for auto-layout. Update DetailSection.test.tsx expectations to match the new responsive classes (sm:grid-cols-2, etc.) so CI reflects the intended behavior.
Mobile detail page has three issues: duplicate sidebar toggle buttons (AppShell + AppHeader both render
SidebarTrigger), action button clutter pushing content down, and single-column field layout wasting horizontal space.Duplicate sidebar toggle
AppShell.tsx: Addhidden md:inline-flextoSidebarTriggerand its adjacent divider — AppHeader already renders its own mobile-only trigger (md:hidden)Action button overflow on mobile
action-bar.tsx: UseuseIsMobile()to reduce inline actions to 1 on mobile (vs 3 on desktop), rest go to overflow menu. NewmobileMaxVisibleschema prop for per-instance control.DetailView.tsx: Hide Share, Edit, Inline Edit buttons on mobile (hidden sm:inline-flex), mirror them assm:hiddenitems in the More Actions dropdown.Detail field grid
DetailSection.tsx: Use responsive Tailwind breakpoints for grid columns instead of hardcoding. Changed fromgrid-cols-1 md:grid-cols-2→grid-cols-1 sm:grid-cols-2, lowering the 2-column threshold frommd(768px) tosm(640px) so more mobile devices get multi-column layout while narrow screens gracefully fall back to 1 column. Progressive breakpoints: 1 col →sm:2 cols →lg:3 cols.Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.