Skip to content

Conversation

nickgros
Copy link
Contributor

Reasons for making this change

[Please describe them here]

If this is related to existing tickets, include links to them as well. Use the syntax fixes #[issue number] (ex: fixes #123).

If your PR is non-trivial and you'd like to schedule a synchronous review, please add it to the weekly meeting agenda: https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit

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.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

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.

Besides moving everything around so that things aren't used before they are defined, I'd say it looks good. IF you do decide to go with a hook (in @rjsf/utils), it probably makes sense to also update my ObjectField refactor to use it too.

"homepage": "https://github.com/rjsf-team/react-jsonschema-form",
"publishConfig": {
"access": "public"
},
Copy link
Member

@heath-freenome heath-freenome Oct 16, 2025

Choose a reason for hiding this comment

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

YAY! This helps a lot

return null;
}

/** Renders a material-ui `GridTemplate` as an item. The `layoutGridSchema` for the `GridType.COLUMN` is separated
Copy link
Member

@heath-freenome heath-freenome Oct 16, 2025

Choose a reason for hiding this comment

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

That material-ui was a left-over from my original implementation. Also consider reformatting the comments to make them flow across lines properly when you remove it

Suggested change
/** Renders a material-ui `GridTemplate` as an item. The `layoutGridSchema` for the `GridType.COLUMN` is separated
/** Renders a `GridTemplate` as an item. The `layoutGridSchema` for the `GridType.COLUMN` is separated

);
}

/** Renders a material-ui `GridTemplate` as an item. The `layoutGridSchema` for the `GridType.COLUMNS` is separated
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Suggested change
/** Renders a material-ui `GridTemplate` as an item. The `layoutGridSchema` for the `GridType.COLUMNS` is separated
/** Renders a `GridTemplate` as an item. The `layoutGridSchema` for the `GridType.COLUMNS` is separated

));
}

/** Renders a material-ui `GridTemplate` as a container. The
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Suggested change
/** Renders a material-ui `GridTemplate` as a container. The
/** Renders a `GridTemplate` as a container. The

* @param [schemaReadonly] - Optional flag indicating whether the schema indicates the field is readonly
* @param [forceReadonly] - Optional flag indicating whether the Form itself is in readonly mode
*/
export function computeFieldUiSchema<T = any, S extends StrictRJSFSchema = RJSFSchema, F extends FormContextType = any>(
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving all the helper functions above the components that are using them...

isReadonly,
optionsInfo,
fieldPathId: fieldIdSchema,
} = getSchemaDetailsForField<T, S, F>(registry, name, initialSchema, formData, fieldPathId);
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
} = getSchemaDetailsForField<T, S, F>(registry, name, initialSchema, formData, fieldPathId);
} = getSchemaDetailsForField<T, S, F>(registry, name, initialSchema, formData, fieldPathId);
const memoFieldPathId = useDeepCompareMemo<FieldPathId>(fieldIdSchema);

schema={schema}
uiSchema={fieldUiSchema}
errorSchema={get(errorSchema, name)}
fieldPathId={fieldIdSchema}
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
fieldPathId={fieldIdSchema}
fieldPathId={memoFieldPathId}

return this.renderField(layoutGridSchema);
}
return <LayoutGridFieldComponent {...props} gridSchema={layoutGridSchema} />;
}
Copy link
Member

Choose a reason for hiding this comment

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

Since my company relies on the LayoutGridField.TEST_IDS variable, can you add the following? Plus that will simplify the tests since you can revert a bunch of changes:

Suggested change
}
}
LayoutGridField.TEST_IDS = LAYOUT_GRID_FIELD_TEST_IDS;

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