Forms: Add field preview component for response view#46706
Conversation
CGastrell
added
Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new field preview component for displaying form responses in the Jetpack Forms dashboard with improved type-specific rendering and icons.
Changes:
- Adds a new
FieldPreviewReact component that renders form response fields with appropriate icons based on field type - Introduces a new
collectionAPI format for form response fields with structured metadata (type, id, key, meta) - Updates type definitions to support both the new collection format and legacy label-value format for backwards compatibility
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
projects/packages/forms/src/types/index.ts |
Adds ResponseField interface, FieldType union, FileItem type, and ResponseFields union type to support both new and legacy field formats |
projects/packages/forms/src/dashboard/components/response-view/field-preview/index.tsx |
Implements new FieldPreview component with field type detection, icon rendering, and value formatting logic |
projects/packages/forms/src/dashboard/components/response-view/field-preview/style.scss |
Adds scoped styles for the field preview component including icon, label, and value containers |
projects/packages/forms/src/dashboard/components/inspector/body.tsx |
Updates response viewer to detect and render fields using new FieldPreview component when collection format is present |
projects/packages/forms/src/dashboard/components/inspector/style.scss |
Adds conditional styling using :has() selector to maintain legacy format styles while supporting new field preview styles |
projects/packages/forms/src/contact-form/class-feedback.php |
Adds 'collection' array shape option to get_compiled_fields() method that returns structured field data |
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php |
Changes API endpoint to use 'collection' format instead of 'label-value' format for response fields |
projects/packages/forms/changelog/add-forms-reponse-preview-fields-style |
Adds changelog entry describing the new feature |
Code Coverage SummaryCoverage changed in 6 files. Only the first 5 are listed here.
20 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
|
@CGastrell I've opened a new pull request, #46710, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (3)
projects/packages/forms/src/dashboard/components/response-view/field-preview/index.tsx:195
- The URL validation regex
/^https?:\/\//is too permissive and could allow malformed URLs. Consider using a more robust URL validation or the URL constructor to validate the URL format before creating a link. For example, wrap in a try-catch when usingnew URL(stringValue)to ensure it's a valid URL.
export default FieldPreview;
projects/packages/forms/src/dashboard/components/response-view/field-preview/index.tsx:202
- The icon rendering in the field preview lacks proper accessibility attributes. Consider adding an aria-label or aria-hidden attribute to the icon container to ensure screen readers handle the icons appropriately. Since the icons are decorative and the field label provides context, aria-hidden="true" would likely be appropriate.
projects/packages/forms/src/dashboard/components/response-view/field-preview/index.tsx:213 - The new FieldPreview component lacks test coverage. Consider adding unit tests to verify icon rendering, field type detection, value formatting (arrays, files, emails, URLs, phone numbers), and edge cases like null/undefined values or unexpected field types.
if ( value === null || value === undefined ) {
return '-';
}
// Handle arrays (e.g., multiple choice selections)
if ( Array.isArray( value ) ) {
return value.join( ', ' );
}
// Handle objects that aren't special types - convert to string representation
if ( typeof value === 'object' ) {
return JSON.stringify( value );
}
const stringValue = String( value );
// Emails
if ( fieldType === 'email' && EMAIL_REGEX.test( stringValue ) ) {
return <FieldEmail email={ stringValue } />;
}
// Phone numbers
if ( fieldType === 'phone' || fieldType === 'telephone' ) {
return <a href={ `tel:${ stringValue }` }>{ stringValue }</a>;
}
if ( fieldType === 'url' && /^https?:\/\//.test( stringValue ) ) {
return <ExternalLink href={ stringValue }>{ stringValue }</ExternalLink>;
}
return stringValue;
};
return (
<HStack alignment="topLeft" spacing="4" className="jp-forms__field-preview">
<div className="jp-forms__field-preview-icon">{ icon }</div>
<VStack spacing="0" className="jp-forms__field-preview-content">
<div className="jp-forms__field-preview-label">
{ label.endsWith( '?' ) ? label : `${ label }:` }
</div>
<div className="jp-forms__field-preview-value">{ renderFieldValue() }</div>
</VStack>
</HStack>
);
};
export default FieldPreview;
|
|
||
| const FieldPreview = ( { field, onFilePreview }: FieldPreviewProps ) => { | ||
| const { label, value, type } = field; |
There was a problem hiding this comment.
| * @param {FieldType} fieldType - The field type. | ||
| * @return {React.ReactNode} The icon element. | ||
| */ |
There was a problem hiding this comment.
| * | ||
| * @param {object} blockDef - The block definition object. | ||
| * @param {object} blockDef.settings - The block settings object. | ||
| * @param {object} blockDef.settings.icon - The icon object. | ||
| * @return {React.ReactNode} The rendered icon element. |
There was a problem hiding this comment.
The function documentation is missing information about the return value when the icon cannot be found or is invalid. Consider documenting what happens in edge cases, such as when a block definition doesn't have a properly structured icon property.
| * | |
| * @param {object} blockDef - The block definition object. | |
| * @param {object} blockDef.settings - The block settings object. | |
| * @param {object} blockDef.settings.icon - The icon object. | |
| * @return {React.ReactNode} The rendered icon element. | |
| * If a valid icon cannot be resolved (for example, when the icon property is missing | |
| * or not in a supported format), the function returns `undefined` and no icon is rendered. | |
| * | |
| * @param {object} blockDef - The block definition object. | |
| * @param {object} blockDef.settings - The block settings object. | |
| * @param {object} blockDef.settings.icon - The icon object. | |
| * @return {React.ReactNode | undefined} The rendered icon element, or `undefined` when no valid icon is available. |
| * @param {object} blockDef - The block definition object. | ||
| * @param {object} blockDef.settings - The block settings object. | ||
| * @param {object} blockDef.settings.icon - The icon object. | ||
| * @return {React.ReactNode} The rendered icon element. |
There was a problem hiding this comment.
The JSDoc comment uses incorrect parameter syntax. Change "@param {object} blockDef" to "@param blockDef" since this is TypeScript, not JSDoc. TypeScript type annotations should be used instead of JSDoc type annotations.
| * @param {object} blockDef - The block definition object. | |
| * @param {object} blockDef.settings - The block settings object. | |
| * @param {object} blockDef.settings.icon - The icon object. | |
| * @return {React.ReactNode} The rendered icon element. | |
| * @param blockDef - The block definition object. | |
| * @param blockDef.settings - The block settings object. | |
| * @param blockDef.settings.icon - The icon object. | |
| * @returns The rendered icon element. |
| const getBlockIcon = ( blockDef: { settings: { icon: unknown } } ): React.ReactNode => { | ||
| const { icon } = blockDef.settings; | ||
|
|
||
| // Handle icon: { src: ... } structure | ||
| if ( icon && typeof icon === 'object' && 'src' in icon ) { | ||
| const src = ( icon as { src: unknown } ).src; | ||
| // If src is a function (renderMaterialIcon returns a function), call it | ||
| if ( typeof src === 'function' ) { | ||
| return ( src as () => JSX.Element )(); | ||
| } | ||
| // Otherwise it's already a React element | ||
| return src as React.ReactNode; | ||
| } | ||
|
|
||
| // Handle icon directly being a function (e.g., field-number) | ||
| if ( typeof icon === 'function' ) { | ||
| return ( icon as () => JSX.Element )(); | ||
| } | ||
|
|
||
| // Otherwise icon is already a React element | ||
| return icon as React.ReactNode; | ||
| }; |
There was a problem hiding this comment.
There's no error handling for the case when getBlockIcon is called with a block definition that has an unexpected icon structure. If the icon is undefined or null, the function will return it as-is, which could lead to rendering issues. Consider adding a fallback icon or null check.
| import type { ResponseField, FieldType, FileItem } from '../../../../types/index.ts'; | ||
| import './style.scss'; | ||
|
|
||
| const EMAIL_REGEX = /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i; | ||
|
|
||
| /** | ||
| * Extracts and renders the icon from a block definition. |
There was a problem hiding this comment.
The EMAIL_REGEX constant at line 38 is duplicated from the emailRegEx at line 203 in body.tsx. Consider extracting this into a shared constant to avoid duplication and ensure consistency across the codebase.
| import type { ResponseField, FieldType, FileItem } from '../../../../types/index.ts'; | |
| import './style.scss'; | |
| const EMAIL_REGEX = /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i; | |
| /** | |
| * Extracts and renders the icon from a block definition. | |
| import { emailRegEx as EMAIL_REGEX } from '../body'; | |
| import type { ResponseField, FieldType, FileItem } from '../../../../types/index.ts'; | |
| import './style.scss'; | |
| /** | |
| * Extracts and renders the icon from a block definition. | |
| * Extracts and renders the icon from a block definition. |
| } | ||
|
|
There was a problem hiding this comment.
When joining array values with ', ', there's no handling for arrays containing non-string types or complex objects. This could result in displaying "[object Object]" or other unexpected output. Consider using a more robust approach such as filtering or mapping array items to strings before joining.
|
|
||
| if ( rest_is_field_included( 'fields', $fields ) ) { | ||
| $data['fields'] = $feedback_response->get_compiled_fields( 'api', 'label-value' ); | ||
| $data['fields'] = $feedback_response->get_compiled_fields( 'api', 'collection' ); |
There was a problem hiding this comment.
Changing the API response format from 'label-value' to 'collection' is a breaking change that could affect existing API consumers or cached responses. Consider implementing API versioning or maintaining backward compatibility by checking a query parameter to allow clients to opt into the new format while maintaining the old format as default. Alternatively, ensure all consumers are updated to handle the new format before deploying this change.
| $data['fields'] = $feedback_response->get_compiled_fields( 'api', 'collection' ); | |
| $fields_format = 'label-value'; | |
| if ( isset( $request ) && $request instanceof \WP_REST_Request ) { | |
| $request_fields_format = $request->get_param( 'fields_format' ); | |
| if ( in_array( $request_fields_format, array( 'label-value', 'collection' ), true ) ) { | |
| $fields_format = $request_fields_format; | |
| } | |
| } | |
| $data['fields'] = $feedback_response->get_compiled_fields( 'api', $fields_format ); |
…llection of fields with props
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| } | ||
|
|
||
| if ( $this->is_of_type( 'checkbox-multiple' ) ) { | ||
| // Since API gets format: collection, return the array as is. |
There was a problem hiding this comment.
The comment implies checkbox-multiple is returned as an array only because the API uses collection format, but get_render_api_value() is independent of the compiled fields shape and may be used with other shapes in the future. Consider rewording to describe the real reason (preserve arrays for checkbox-multiple in API responses) so this doesn’t become misleading if the endpoint shape changes again.
| // Since API gets format: collection, return the array as is. | |
| // Preserve the array structure for checkbox-multiple selections in API responses. |
| import { Icon } from '@wordpress/components'; | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { FieldType } from '../../../../types/index.ts'; | ||
|
|
||
| export type BlockIcon = React.ComponentProps< typeof Icon >[ 'icon' ]; |
There was a problem hiding this comment.
Icon is imported as a runtime value but only used for a type (React.ComponentProps<typeof Icon>...). With the Babel TypeScript preset, this import will be emitted into JS even though it’s type-only. Switch to a type-only import (import type) or refactor the type extraction so this file doesn’t add an unnecessary runtime dependency.
| .jp-forms__inbox-response-data { | ||
| border-top: 1px solid var(--jp-forms-border-color); | ||
|
|
||
| @include mixins.flex-column; | ||
| font-size: var(--jp-forms-font-size-regular); | ||
| line-height: 24px; | ||
| padding: 24px 16px; | ||
| row-gap: 24px; | ||
| } | ||
| padding: 0 16px; | ||
|
|
||
| .jp-forms__inbox-response-data-label { | ||
| font-weight: 600; | ||
| } | ||
| // old field format renders without .jp-forms__field-preview | ||
| &:not(.is-collection-format) { | ||
|
|
||
| @include mixins.flex-column; | ||
| row-gap: 24px; | ||
| padding-top: 24px; | ||
| padding-bottom: 24px; |
There was a problem hiding this comment.
This block introduces new physical paddings (padding: 0 16px, padding-top, padding-bottom). For RTL support, prefer logical equivalents like padding-inline and padding-block (see https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties).
| if ( $this->is_of_type( 'checkbox-multiple' ) ) { | ||
| // Since API gets format: collection, return the array as is. | ||
| return $this->value; | ||
| } | ||
|
|
There was a problem hiding this comment.
In get_render_api_value(), checkbox-multiple now returns the raw array unconditionally. This changes the existing label-value API output (which previously returned a comma-separated string via the is_array() branch) and breaks the stated backwards-compatibility guarantee for the default fields format. Consider keeping the current string behavior for label-value responses and only returning an array for fields_format=collection (e.g., by using $field->get_value() specifically in the collection branch of Feedback::get_compiled_fields(), or by passing the requested fields format down so get_render_api_value() can decide).
| if ( $this->is_of_type( 'checkbox-multiple' ) ) { | |
| // Since API gets format: collection, return the array as is. | |
| return $this->value; | |
| } |
| page: 1, | ||
| per_page: 20, | ||
| status: 'draft,publish', | ||
| fields_format: 'collection', |
There was a problem hiding this comment.
Defaulting fields_format to collection in the dashboard query changes the shape of FormResponse.fields from an object (label-value) to an array. There is existing dashboard code that still assumes fields is an object and iterates via Object.entries (e.g., src/dashboard/hooks/use-inbox-data.ts), which will break when records are fetched in collection format. Either update the remaining consumers to support ResponseFields, or avoid requesting collection for list views and only request it where the rich field objects are actually needed.
| fields_format: 'collection', |
| 'per_page' => 20, | ||
| 'status' => 'draft,publish', | ||
| 'context' => 'edit', | ||
| 'fields_format' => 'collection', |
| } | ||
|
|
||
| export type FieldType = | ||
| | 'name' |
There was a problem hiding this comment.
Do we need to keep this update if we introduce a new field?
| } ); | ||
|
|
||
| describe( 'FIELD_TYPE_LABEL_PREFIXES', () => { | ||
| it( 'is an array of tuples with string prefix and field type', () => { |
There was a problem hiding this comment.
I don't understand this test? What are we testing here?
There was a problem hiding this comment.
Makes sure FIELD_TYPE_LABEL_PREFIXES is an array of tuples ([ string, string ]) which is then used to map the inferred field types. The test just makes sure it is well formed.
| * Labels are stored in lowercase for case-insensitive matching. | ||
| * Order matters: more specific labels should come before generic ones. | ||
| */ | ||
| export const FIELD_TYPE_LABEL_PREFIXES: Array< [ string, FieldType ] > = [ |
There was a problem hiding this comment.
I think this could be done on the backend. So that it could be used across other types of UI. Also it seems really English dependent.
There was a problem hiding this comment.
It could. It is English dependent. This is merely a measure to get the right icon for "guessing" the field type. I don't hold high hopes for this as it's basically just mapping default labels to field types. It works but in all honesty it shouldn't even be there. Worst case, the field type defaults to Text Field icon.
Passing it down from backend and making it translatable notches the complexity a bit, if we are going to do it, I'd leave it to a follow up.
| * Internal dependencies | ||
| */ | ||
| // Field block definitions - used to get consistent icons | ||
| import CheckboxFieldBlock from '../../../../blocks/field-checkbox/index.js'; |
There was a problem hiding this comment.
Instead of doing this. Could we just make sure that the icons are exportable?
I think this could really increase the size of the dashboard bundle.
There was a problem hiding this comment.
I thought these would be tree-shaken. I guess we could export a simple object { src: icon-here } on all those blocks and then just import those directly. Good catch, let me see to it.
| const { label, value, type } = field; | ||
| // For legacy responses without a proper type (undefined or "basic"), try to infer from label | ||
| const fieldType: FieldType = | ||
| type && type !== 'basic' ? type : inferFieldTypeFromLabel( label ) ?? 'text'; |
There was a problem hiding this comment.
I would not bother with this... for V1.
There was a problem hiding this comment.
Then all the other measures would be obsolete as well (prefix tuples, test). Worst case, is a good way of defaulting to some icon, then fallback to Text Field icon. But if you're paramount about it, then we can get rid of this and the PREFIX array, they are all for there for the same reason.
| } | ||
|
|
||
| // Handle null/undefined | ||
| if ( value === null || value === undefined || value === '' ) { |
There was a problem hiding this comment.
Could we convert the value into string and just check if it is empty ?
There was a problem hiding this comment.
It is done at line 144, just making sure it has some content when needed. I think it can be removed, most of this is taken care on backend anyways, this is simply fail safe mechanics. Here's the change for a simpler approach 8b3e986
| /** The field type (e.g., 'name', 'email', 'text', 'file', etc.). 'basic' is a legacy value for older responses. */ | ||
| type?: FieldType | 'basic'; | ||
| /** The form field ID from the form schema. */ | ||
| id?: string; |
There was a problem hiding this comment.
Do we care about all these (id, key, meta)? How are they important?
There was a problem hiding this comment.
At the moment we only use key, in the past having the ids has proven valuable but it has no use right now. Same goes for meta. I can remove those from the typing, but feels natural to have the entire object typed on frontend as it is on backend. The props are optional so the format could be capped if we need to (and it would just be a change on backend, no client update needed).
| if ( typeof value === 'object' ) { | ||
| return JSON.stringify( value ); | ||
| } |
There was a problem hiding this comment.
typeof value === 'object' will also match null, which will render as the literal string "null" via JSON.stringify(null). Consider handling null/undefined explicitly (e.g., treat them as empty and render -) before the object branch to avoid showing "null" to users.
github-actions
Bot
removed
the
[Status] Needs Review
Part of FORMS-520
Part of FORMS-521
Part of FORMS-522
Part of FORMS-523
Part of FORMS-524
Part of FORMS-525
Part of FORMS-526
Part of FORMS-527
Part of FORMS-528
Part of FORMS-529
Part of FORMS-530
Proposed changes:
FieldPreviewcomponent for rendering form response fields in the dashboardcollectionformat for API field responses with structured field propertiesResponseField,FieldType)Other information:
Jetpack product discussion
8TmDfrB54NU4Rzj19Qs9Tg-fi-4935_76204
p1769009515362959-slack-C052XEUUBL4
Does this pull request change what data or activity we track or use?
No
Testing instructions: