feat(component): Field builder component #777

parthivrh · 2025-07-29T18:23:35Z

Field builder component. Closes #731

patternfly-build · 2025-07-29T18:24:48Z

Preview: https://react-component-groups-pr-component-groups-777.surge.sh

A11y report: https://react-component-groups-pr-component-groups-777-a11y.surge.sh

thatblindgeye

Some initial comments below, let me know if you have any questions/concerns

thatblindgeye · 2025-07-29T18:38:35Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+ * This allows each rendered row to have access to its own specific index.
+ */
+export interface FieldRowHelpers {
+  rowIndex: number;


Would we expect to be able to pass in rowIndex for any reason?

thatblindgeye · 2025-07-29T18:45:58Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+   * Unique ID for this row group - use this for aria-labelledby associations
+   */
+  rowGroupId: string;


This really applies to all the props that are creating or being used in some way for the aria-labelledby on the inputs and such, but an alternative would be to use aria-label instead.

The pro to that is it's less content that needs to be rendered in the DOM mainly, possibly another pro is AT won't traverse to the visually hidden text (which whether it's a pro depends on whether it'd be something users wwould find helpful, so we don't really know at this time)

Either way probably makes sense, since we're dealing with a mixture of visible text and non-visible text to reference.

thatblindgeye · 2025-07-29T19:07:35Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+   * Complete aria-labelledby string for the first column that includes both row and column context
+   */
+  firstColumnAriaLabelledBy: string;
+  /**
+   * Complete aria-labelledby string for the second column that includes both row and column context
+   */


If we do want to keep the aria-labelledby approach, we should just update these descriptions so that it's clear that a space separated list of IDs referencing other elements must be passed in here, and that doing so allows custom labeling/overriding the default aria-labelledby

thatblindgeye · 2025-07-29T19:10:08Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+  /** A callback triggered when the "Add" button is clicked. */
+  onAddRow: () => void;


Can we include the event object as a param here?

thatblindgeye · 2025-07-29T19:10:18Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+  /** A callback triggered when the "Add" button is clicked. */
+  onAddRow: () => void;
+  /** A callback triggered when a "Remove" button is clicked, which receives the index of the row to remove. */
+  onRemoveRow: (index: number) => void;


Same as above here, with event being the first param

thatblindgeye · 2025-07-29T19:25:00Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+  // Enhanced onAddRow with focus management and announcements
+  const handleAddRow = useCallback(() => {
+    onAddRow();
+    announceChange(`New ${rowGroupLabelPrefix.toLowerCase()} added. ${rowGroupLabelPrefix} ${rowCount + 1}.`);


We should allow some way to customize what can be passed into this announceChange callback for both adding and removing a row. On adding a row a consumer may only want to say that a new row was added, rather than what the row number is. On removing a row, it may make sense to reference the "primary" input value (e.g. "Removed row John Smith").

thatblindgeye · 2025-07-29T19:27:13Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+  }, [ onAddRow, announceChange, rowGroupLabelPrefix, rowCount ]);
+
+  // Enhanced onRemoveRow with focus management and announcements
+  const handleRemoveRow = useCallback((index: number) => {


For the logic here, I'm thinking that we shouldn't move focus to another destructive action, especially since there's no confirmation when removing a row. Rather than moving to another row's "remove" button, can we move focus to the first input in that row perhaps?

Though another thought I suppose is whether we should be providing this logic or if it should be up to the consumer. We don't handle any logic for adding a row - that's done in the examples. Maybe input from others on whether this sort of logic should be handled by component-groups.

thatblindgeye · 2025-07-29T19:31:50Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+
+  // Enhanced onAddRow with focus management and announcements
+  const handleAddRow = useCallback(() => {
+    onAddRow();


Depending on the resolution of the above comment, either in the component code or example code, when a new row is added we should shift focus to that new rows first input.

thatblindgeye · 2025-07-29T19:33:21Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+            icon={<PlusCircleIcon />} 
+            {...addButtonProps}
+          >
+            {addButtonProps.children || 'Add another'}


Nit: we could add a "addButtonContent" prop instead of having to pass addButtonProps.children inside an object.

thatblindgeye · 2025-07-29T19:33:57Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+            <Button
+              ref={createRemoveButtonRef(index)}
+              variant="plain"
+              aria-label={`Remove ${rowGroupLabelPrefix.toLowerCase()} ${rowNumber}`}


We should also allow customizing this aria-label further with a prop.

thatblindgeye

Another batch of comments below.

Additionally, I've been thinking about it more, and using a Table for all of this might make a bit more sense to help link relationships between things.

Right now, traversing with VO, the "name" and "email" texts at the top of the columns are pretty standalone - there's really nothing stating what they're for or any relationship. So it can kinda give off a "name and email for what?" vibes. It's not really until you navigate to the inputs (assuming there are any) that it may become clearer what those texts mean/are for. Visually it works, but from an AT standpoint it may be less clear.

Using a Table would help link those relationships more clearly and immediately, since those would be proper column headers for something. You'll know "okay this is a Name column for some stuff".

Would you be willing to try building this out using a Table instead of just Grid layouts?

thatblindgeye · 2025-08-01T13:33:30Z

...rnfly-docs/content/extensions/component-groups/examples/FieldBuilder/FieldBuilderExample.tsx

+        const addButton = document.querySelector('button[aria-label*="Add"]') as HTMLButtonElement;
+        if (addButton) {
+          addButton.focus();
+        }


This isn't being ran since there's no button with an aria-label of "Add". If we wanted to keep the querySelector we could add a unique ID to the add button and target that.

thatblindgeye · 2025-08-01T13:40:01Z

...rnfly-docs/content/extensions/component-groups/examples/FieldBuilder/FieldBuilderExample.tsx

+    setContacts(newContacts);
+
+    // Focus management: avoid focusing on destructive actions
+    setTimeout(() => {


Two points for all the focus logic in these examples:

Spoke with Nicole this morning, and it'd be great if we can bake this focus logic inside the FieldBuilder rather than consumers needing to pass all this logic in. Maybe default logic that handles the typical use-case, with these onRemoveRow and onAddRow props potentially being a means for consumers to pass in their own custom logic if they need to.

This is really more of a nit and we can do it in a followup to investigate a bit more, but if possible it'd be great to not rely on a timeout for the focus logic. Would probably involve using a useEffect and having to track the previous length of newContacts to compare to the current length (tell if something was added or removed), etc. I'm personally always a bit wary of using timeouts for this sort of thing (even though I will use them if necessary), just because there may not be a guarantee the focus will occur after the specified timeout length, plus other side effects (for example, when I remove a row on the PR preview, the focus goes to the main page area briefly before moving to the correct row's input - so I VoiceOver starts announcing "PatternFly field builder, content", but gets cut off once focus does go to the input).

Not a blocker right now, so if you would prefer opening a followup for this I'd be good with that.

thatblindgeye · 2025-08-01T14:01:33Z

...rnfly-docs/content/extensions/component-groups/examples/FieldBuilder/FieldBuilderExample.tsx

+        onAddRowAnnouncement={customAddAnnouncement}
+        onRemoveRowAnnouncement={customRemoveAnnouncement}
+        removeButtonAriaLabel={customRemoveAriaLabel}
+        addButtonContent="Add team member"


Let's give these examples more unique add button texts.

thatblindgeye · 2025-08-01T14:04:53Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+   * Ref callback to attach to the first focusable element in the row.
+   * This enables automatic focus management when rows are added/removed.
+   */
+  focusRef: (element: HTMLElement | null) => void;


Does this ref actually get used for anything currently? The examples are using querySelectors and such to handle focusing logic so just curious.

thatblindgeye · 2025-08-01T14:45:06Z

packages/module/src/FieldBuilder/FieldBuilder.tsx

+          aria-atomic="true"
+          role="status"


We can remove these 2 attributes.

aria-atomic isn't really relevant here since there's only really going to be a text string rendered inside this div. aria-atomic would be more helpful for something like a div that contained "7 messages", with "7" and "messages" being in their own <span> elements. By default a live region on the div would only announce when the number changes which most likely isn't super helpful - adding aria atomic would help ensure that what gets announced is "8 messages" rather han just "8" for example.

the role of status is basically the same as setting aria-live of polite and aria-atomic of true. Since we don't need aria-atomic here, we can just remove the role as well.

kaylachumley

Hey! Just a few things:

Can you please adjust the spacing/padding between the input fields to be 8px padding (pf-t--global--spacer--sm)?
Can you please adjust the padding between the input fields and the input field labels to be 8px padding (pf-t--global--spacer--sm)?
Can you change the inline link button to be a link button(no underline)? dont worry about the left alignment
Can you please adjust the padding between the input fields and the link button to be 8px (pf-t--global--spacer--sm)?

thatblindgeye reviewed Jul 29, 2025

View reviewed changes

andrew-ronaldson requested a review from kaylachumley August 1, 2025 14:09

thatblindgeye reviewed Aug 1, 2025

View reviewed changes

kaylachumley suggested changes Aug 1, 2025

View reviewed changes

parthivrh added 4 commits August 5, 2025 13:52

feat(component): Field builder component using grid layout

663c60e

fix: incorporated accessibility feedback

aeb7b7a

fix: added column labels

eda5df9

fix: switched from grid to table

7deaff0

parthivrh force-pushed the field-builder branch from 2daea20 to 7deaff0 Compare August 5, 2025 18:42

		/** A callback triggered when the "Add" button is clicked. */
		onAddRow: () => void;

feat(component): Field builder component #777

Are you sure you want to change the base?

feat(component): Field builder component #777

Conversation

parthivrh commented Jul 29, 2025

Uh oh!

patternfly-build commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaylachumley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patternfly-build commented Jul 29, 2025 •

edited

Loading