Skip to content

Conversation

JJediny
Copy link

@JJediny JJediny commented May 2, 2025

Closes #4573

Reasons for making this change

This PR introduces a new theme package, @rjsf/uswds, implementing the U.S. Web Design System (USWDS) v3 for react-jsonschema-form.

Many government projects require adherence to USWDS guidelines. This theme provides developers with an RJSF implementation that uses standard USWDS components, classes, and structure, ensuring visual consistency and compliance.

Key features include:

  • Templates for all standard RJSF fields (Object, Array, String, Number, Boolean, etc.) styled according to USWDS.
  • Widgets leveraging USWDS form controls (usa-input, usa-textarea, usa-checkbox, usa-radio, usa-select, etc.).
  • Array and Object field templates using USWDS fieldset, legend, and grid layout (grid-row, grid-col).
  • Action buttons (Add, Remove, Move Up, Move Down) using USWDS button styles (usa-button, usa-button--outline, usa-button--unstyled) and icons loaded from the official USWDS CDN.
  • Error display using USWDS error states (usa-form-group--error, usa-label--error, usa-error-message).
  • Required field indication using usa-label--required.
  • Refactored button implementation for better maintainability.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update to update snapshots, if needed. (Self-verification step)
    • I've updated docs if needed (Added theme README)
    • I've updated the changelog with a description of the PR (Self-verification step - Add an entry like: feat(@rjsf/uswds): Add new theme package for US Web Design System v3)
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@JJediny JJediny mentioned this pull request May 2, 2025
8 tasks
# Dependency directories
node_modules/

# Optional npm cache directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already in the .gitignore on the head of main which you are not on yet

{...otherProps}
data-testid="copy-button"
aria-label={translatedLabel}
className={`usa-button usa-button--unstyled ${otherProps.className || ''}`.trim()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
className={`usa-button usa-button--unstyled ${otherProps.className || ''}`.trim()}
className={`usa-button usa-button--unstyled ${className}`.trim()}

S extends StrictRJSFSchema = RJSFSchema,
F extends FormContextType = any,
>(props: IconButtonProps<T, S, F>) {
const { icon, iconType, registry, ...otherProps } = props; // Destructure registry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { icon, iconType, registry, ...otherProps } = props; // Destructure registry
const { icon, iconType, registry, className = '', ...otherProps } = props; // Destructure registry

const translatedLabel = registry.translateString(TranslatableString.AddItemButton);
return (
// Add appropriate styling for an icon-only button if desired, e.g., usa-button--icon-only
<Button type="button" {...otherProps} data-testid="add-button" aria-label={translatedLabel} className={`usa-button usa-button--outline ${otherProps.className || ''}`.trim()}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Button type="button" {...otherProps} data-testid="add-button" aria-label={translatedLabel} className={`usa-button usa-button--outline ${otherProps.className || ''}`.trim()}>
<Button type="button" {...otherProps} data-testid="add-button" aria-label={translatedLabel} className={`usa-button usa-button--outline ${className}`.trim()}>


export default function DescriptionField<T = any, S extends RJSFSchema = RJSFSchema, F extends FormContextType = any>({
description,
id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id,
id,
registry,
uiSchema,

@heath-freenome
Copy link
Member

Still more comments to come

Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JJediny I'm not going to pull any punches here. This theme is a mess. Tons of duplicated code, implementing things that aren't necessary, multiple template implementations for the same thing. It seems like you don't really understand the basic concepts of building a theme that extends the core theme. Most theme builders look at the other non-core themes that closest represent the way their theme library works, copy that and change the templates and widgets. No need for fields to be updated at all.

I'm almost tempted to suggest you restart building your theme by copying something like the react-bootstrap theme as the starting point and reimplement. There is a lot of cleanup work you'll need to do so that you can have this theme ready

Comment on lines +35 to +37
},
},
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert these changes?

"@babel/preset-react",
"@babel/preset-typescript"
]
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The little dash surrounded by the red circle means this needs a blank line

Suggested change
}
}

Comment on lines +13 to +15
"require": "./dist/index.js",
"import": "./lib/index.js",
"types": "./lib/index.d.ts"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, the types are supposed to be first, my bad

Suggested change
"require": "./dist/index.js",
"import": "./lib/index.js",
"types": "./lib/index.d.ts"
"types": "./lib/index.d.ts",
"require": "./dist/index.js",
"import": "./lib/index.js"

Comment on lines +18 to +20
"require": "./dist/index.js",
"import": "./lib/index.js",
"types": "./lib/index.d.ts"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"require": "./dist/index.js",
"import": "./lib/index.js",
"types": "./lib/index.d.ts"
"types": "./lib/index.d.ts",
"require": "./dist/index.js",
"import": "./lib/index.js"

Comment on lines +23 to +25
"require": "./dist/*.js",
"import": "./lib/*.js",
"types": "./lib/*.d.ts"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"require": "./dist/*.js",
"import": "./lib/*.js",
"types": "./lib/*.d.ts"
"types": "./lib/*.d.ts",
"require": "./dist/*.js",
"import": "./lib/*.js"

@@ -0,0 +1,19 @@
import Form from '../src';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this, please delete the whole file

@@ -0,0 +1,22 @@
import Form from '../src';
// import { createSchemaTest } from '../schemaTests'; // Removed usage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this, please delete the whole file

Comment on lines +7 to +12
// Create a simple test suite to prevent "test suite must contain at least one test" error
describe('ArrayField', () => {
it('should have a test', () => {
expect(true).toBe(true);
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need this as the other themes don't complain

Comment on lines +7 to +13
// Create a simple test suite to prevent "test suite must contain at least one test" error
describe('Form', () => {
it('should have a test', () => {
expect(true).toBe(true);
});
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need this as the other themes don't complain

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a .jsc file, use .json instead

@JJediny JJediny closed this May 7, 2025
@heath-freenome
Copy link
Member

heath-freenome commented May 8, 2025

@JJediny I apologize if my feedback felt harsh. I'm hoping you closed this to follow my suggestion of starting fresh by modifying a copied theme rather than giving up on adding the theme entirely. Feel free to reach out on discord if you need any help

@JJediny
Copy link
Author

JJediny commented May 8, 2025

@heath-freenome all good, yes will resume work on our fork and better formulate before another PR. This dropped on our immediate priorities. Next time I will be sure to address all the feedback and suggestions thus far and start from an established theme. Appreciate your time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants