Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…mmaryFields, responsive fix, collapseWhenEmpty - Add hideEmpty to DetailViewSection type (packages/types) - Add primaryField and summaryFields to DetailViewSchema type - Implement hideEmpty field filtering in DetailSection with empty section hiding - Fix responsive breakpoints: sm:grid-cols-2 → md:grid-cols-2, md:grid-cols-3 → lg:grid-cols-3 - Render data[primaryField] as header title with Badge rendering for summaryFields - Add collapseWhenEmpty prop to RecordActivityTimeline - RecordDetailView: read form sections from objectDef, remove columns: 2 hardcode, auto-detect primaryField - Add 11 new tests covering all new features Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…tion Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…bjectDef.primaryField, warn on missing fields Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 仔细评估是否已经完成了任务中的所有要求 |
…oolean warning badge - EmailCellRenderer: add hover copy-to-clipboard button alongside mailto link - PhoneCellRenderer: add phone icon and hover copy-to-clipboard button - BooleanCellRenderer: render destructive warning Badge for active/enabled/verified=false - Update existing tests for new boolean behavior, add 12 new field renderer tests - Update ROADMAP.md with field renderer enhancements Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…abel fallback, add clipboard .catch() - Extract STATUS_FIELD_NAMES Set for maintainability - Fix label fallback to use fieldName variable instead of 'Inactive' - Add .catch() to clipboard operations for insecure contexts Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Reviewed the full requirement list and found item 6 (字段渲染器增强) was missing. Now completed in
All 6 items from the issue are now complete:
|
There was a problem hiding this comment.
Pull request overview
Enhances the DetailView experience across the platform by better consuming the existing view schema capabilities (sections, responsive layout, header semantics) and improving common field renderers, with corresponding test + Storybook coverage.
Changes:
- Extend
@object-ui/typesDetailView schemas withhideEmpty,primaryField, andsummaryFields. - Update
@object-ui/plugin-detailrendering: hide empty fields/sections, improve header title + badges, and adjust responsive breakpoints; addcollapseWhenEmptybehavior to activity timeline. - Enhance
@object-ui/fieldscell renderers (email/phone copy UX, boolean status warning badge) and update console’s RecordDetailView to respect form section grouping + inferred layout.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/views.ts | Adds new DetailView schema surface (hideEmpty, primaryField, summaryFields). |
| packages/plugin-detail/src/DetailSection.tsx | Implements hide-empty filtering and updates grid breakpoint classes. |
| packages/plugin-detail/src/DetailView.tsx | Renders primary-field header title and summary-field badges. |
| packages/plugin-detail/src/RecordActivityTimeline.tsx | Adds collapseWhenEmpty option to suppress empty-state message. |
| apps/console/src/components/RecordDetailView.tsx | Generates grouped sections from objectDef.views.form.sections and infers primaryField. |
| packages/fields/src/index.tsx | Adds email/phone copy affordances and boolean status “Off” warning badge behavior. |
| packages/plugin-detail/src/tests/autoLayout.test.ts | Adds regression coverage for undefined columns. |
| packages/plugin-detail/src/tests/DetailSection.test.tsx | Updates breakpoint assertions and adds hideEmpty behavior tests. |
| packages/plugin-detail/src/tests/DetailView.test.tsx | Adds tests for primaryField and summaryFields header rendering. |
| packages/plugin-detail/src/tests/RecordActivityTimeline.test.tsx | Adds test coverage for collapseWhenEmpty. |
| packages/fields/src/tests/cell-renderers.test.tsx | Adds/updates tests for new email/phone renderers + boolean warning badge. |
| packages/fields/src/tests/boolean-checkbox.test.tsx | Adjusts checkbox tests to account for status-field warning badge behavior. |
| packages/plugin-detail/src/DetailView.stories.tsx | Adds stories demonstrating primaryField/badges and hideEmpty sections. |
| ROADMAP.md | Updates roadmap with March 2026 DetailView optimization entry. |
| return value !== null && value !== undefined && value !== ''; | ||
| }) | ||
| : section.fields; | ||
|
|
There was a problem hiding this comment.
When hideEmpty filters out fields, the section header’s field-count Badge still uses section.fields.length (original count) elsewhere in this component, which will become inaccurate. Consider switching the count display to use the filtered visibleFields.length when hideEmpty is enabled.
| // When hideEmpty is enabled, ensure any field-count display based on | |
| // section.fields.length reflects the filtered set of visible fields. | |
| if (section.hideEmpty && section.fields !== visibleFields) { | |
| section.fields = visibleFields; | |
| } |
| </Button> | ||
| <button | ||
| type="button" | ||
| className="opacity-0 group-hover/email:opacity-100 transition-opacity p-0.5 rounded hover:bg-muted" |
There was a problem hiding this comment.
The copy button is only revealed on hover (opacity-0 + group-hover/...:opacity-100), which makes it hard/impossible to discover via keyboard navigation. Add focus styling (e.g. focus-visible:opacity-100) and ensure it’s reachable/visible when tabbed to.
| className="opacity-0 group-hover/email:opacity-100 transition-opacity p-0.5 rounded hover:bg-muted" | |
| className="opacity-0 group-hover/email:opacity-100 focus-visible:opacity-100 transition-opacity p-0.5 rounded hover:bg-muted focus-visible:bg-muted focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background" |
| navigator.clipboard.writeText(String(value)).then(() => { | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }).catch(() => { /* clipboard not available */ }); |
There was a problem hiding this comment.
Same issue as EmailCellRenderer: navigator.clipboard.writeText(...) is called without checking navigator.clipboard exists, so it can throw in insecure contexts before .catch() runs. Guard before calling.
| navigator.clipboard.writeText(String(value)).then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }).catch(() => { /* clipboard not available */ }); | |
| try { | |
| const clipboard = navigator.clipboard; | |
| if (!clipboard || typeof clipboard.writeText !== 'function') { | |
| // Clipboard API is not available in this context | |
| return; | |
| } | |
| clipboard | |
| .writeText(String(value)) | |
| .then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }) | |
| .catch(() => { | |
| // Clipboard write failed | |
| }); | |
| } catch { | |
| // Accessing navigator.clipboard is not allowed in this context | |
| } |
| const { container } = render(<DetailView schema={schema} />); | ||
| // The header area should have a badge for 'Active' but not 'department' | ||
| // Find badges within the header | ||
| const headerBadges = container.querySelectorAll('.border-b .rounded-full'); | ||
| const badgeTexts = Array.from(headerBadges).map(b => b.textContent); | ||
| expect(badgeTexts).toContain('Active'); |
There was a problem hiding this comment.
This assertion relies on internal styling/classnames (.border-b .rounded-full) to locate header badges, which is brittle against styling refactors. Since badges already have aria-label in the implementation, prefer querying by accessible name/role (or a testid) scoped to the header.
| const { container } = render(<DetailView schema={schema} />); | |
| // The header area should have a badge for 'Active' but not 'department' | |
| // Find badges within the header | |
| const headerBadges = container.querySelectorAll('.border-b .rounded-full'); | |
| const badgeTexts = Array.from(headerBadges).map(b => b.textContent); | |
| expect(badgeTexts).toContain('Active'); | |
| render(<DetailView schema={schema} />); | |
| // The header area should have a badge for the non-empty status but not for the empty department | |
| const statusBadge = screen.getByLabelText('Status: Active'); | |
| expect(statusBadge).toBeInTheDocument(); | |
| expect(screen.queryByLabelText(/Department:/i)).toBeNull(); |
| {/* Timeline */} | ||
| {filtered.length === 0 ? ( | ||
| <p className="text-sm text-muted-foreground text-center py-4"> | ||
| No activity recorded | ||
| </p> | ||
| collapseWhenEmpty ? null : ( | ||
| <p className="text-sm text-muted-foreground text-center py-4"> | ||
| No activity recorded | ||
| </p> | ||
| ) |
There was a problem hiding this comment.
collapseWhenEmpty is documented as “collapse to only the comment input when there are no items”, but the implementation only hides the "No activity recorded" message; the CardHeader/filter chrome still renders. Either update the prop documentation to match behavior or extend the conditional rendering to actually collapse the header/filter/content when empty.
| navigator.clipboard.writeText(String(value)).then(() => { | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }).catch(() => { /* clipboard not available */ }); |
There was a problem hiding this comment.
handleCopy calls navigator.clipboard.writeText(...) without guarding for navigator.clipboard being undefined. In insecure contexts or older browsers this throws before the Promise .catch() runs. Add an existence check or wrap the call in try/catch before attempting to write.
| navigator.clipboard.writeText(String(value)).then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }).catch(() => { /* clipboard not available */ }); | |
| if (typeof navigator === 'undefined' || !navigator.clipboard || typeof navigator.clipboard.writeText !== 'function') { | |
| // Clipboard API is not available in this environment | |
| return; | |
| } | |
| try { | |
| navigator.clipboard | |
| .writeText(String(value)) | |
| .then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }) | |
| .catch(() => { | |
| // Clipboard write failed or is not permitted | |
| }); | |
| } catch { | |
| // Accessing navigator.clipboard or calling writeText threw synchronously | |
| } |
| </a> | ||
| <button | ||
| type="button" | ||
| className="opacity-0 group-hover/phone:opacity-100 transition-opacity p-0.5 rounded hover:bg-muted" |
There was a problem hiding this comment.
The phone copy button is hover-only (opacity-0 + group-hover/...:opacity-100), which is not keyboard-discoverable. Add focus-visible (and/or always-visible on keyboard focus) so keyboard users can find and use it.
| className="opacity-0 group-hover/phone:opacity-100 transition-opacity p-0.5 rounded hover:bg-muted" | |
| className="opacity-0 group-hover/phone:opacity-100 focus-visible:opacity-100 transition-opacity p-0.5 rounded hover:bg-muted focus-visible:bg-muted focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background" |
| fields: (sec.fields || []).map((f: any) => { | ||
| const fieldName = typeof f === 'string' ? f : f.name; | ||
| const fieldDef = objectDef.fields[fieldName]; | ||
| if (!fieldDef) { | ||
| console.warn(`[RecordDetailView] Field "${fieldName}" not found in ${objectDef.name} definition`); | ||
| return { name: fieldName, label: fieldName }; | ||
| } |
There was a problem hiding this comment.
In the form-sections path, objectDef.fields[fieldName] assumes objectDef.fields is always defined, but this file elsewhere treats it as optional (objectDef.fields || {}). If metadata can omit fields, this will throw. Use a safe fallback (e.g. objectDef.fields?.[fieldName]) and ensure the missing-field fallback still provides a default type so downstream renderers behave consistently.
| <h1 className="text-xl sm:text-2xl font-bold truncate"> | ||
| {(schema.primaryField && data?.[schema.primaryField]) || schema.title || 'Details'} | ||
| </h1> |
There was a problem hiding this comment.
Header title uses (schema.primaryField && data?.[schema.primaryField]) || ..., which treats valid falsy values (e.g. 0/false) as “empty” and incorrectly falls back to schema.title. Prefer an explicit emptiness check (null/undefined/empty string) before falling back.
| <h1 className="text-xl sm:text-2xl font-bold truncate"> | |
| {(schema.primaryField && data?.[schema.primaryField]) || schema.title || 'Details'} | |
| </h1> | |
| {(() => { | |
| const primaryFieldValue = schema.primaryField ? data?.[schema.primaryField] : undefined; | |
| const hasPrimaryFieldValue = | |
| primaryFieldValue !== null && | |
| primaryFieldValue !== undefined && | |
| primaryFieldValue !== ''; | |
| const headerTitle = | |
| (hasPrimaryFieldValue ? String(primaryFieldValue) : undefined) ?? | |
| schema.title ?? | |
| 'Details'; | |
| return ( | |
| <h1 className="text-xl sm:text-2xl font-bold truncate"> | |
| {headerTitle} | |
| </h1> | |
| ); | |
| })()} |
Platform DetailView renderer under-consumes existing spec capabilities, resulting in flat single-section layouts, static headers showing object type instead of record name, wasted space from empty fields, and premature multi-column breakpoints on tablet viewports.
Types (
@object-ui/types)DetailViewSection.hideEmpty?: boolean— filter null/undefined/empty-string fields; hide section when all filteredDetailViewSchema.primaryField?: string— resolve header title fromdata[primaryField], fallback totitleDetailViewSchema.summaryFields?: string[]— render as<Badge>beside header titleDetailSection
hideEmptyfiltering before auto-layout,return nullon empty sectionsm:grid-cols-2→md:grid-cols-2,md:grid-cols-3→lg:grid-cols-3(iPad+sidebar no longer triggers multi-column prematurely)DetailView header
data[primaryField]as<h1>withsummaryFieldsas accessible<Badge aria-label>elementsRecordActivityTimeline
collapseWhenEmptyprop suppresses "No activity recorded" chrome, shows only comment inputRecordDetailView (console)
objectDef.views?.form?.sectionsfor section grouping; fallback to flat field listcolumns: 2hardcode —autoLayoutinfers from field countprimaryFieldfromobjectDef.primaryFieldmetadata, thenname/titleheuristicField Renderer Enhancements (
@object-ui/fields)mailto:link + hover copy-to-clipboard buttontel:link with call icon + hover copy-to-clipboard buttonBadgefor status fields (active,is_active,enabled,is_enabled,verified,is_verified) when value isfalse(e.g. "Active — Off");STATUS_FIELD_NAMESextracted as maintainableSetconstant.catch()handlers for insecure contextsTests
hideEmptyfiltering, empty section hiding,primaryField/summaryFieldsrendering, responsive breakpoint class assertions,collapseWhenEmpty,autoLayoutundefined-columns regressionStorybook
PrimaryFieldWithBadgesandHideEmptyFieldsstoriesOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.