Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bootstrap-4 TextWidget wrappers now pull from registry, add rootSchema to Registry, Fix FieldProps.onFocus type to match WidgetProps #2519

Merged
merged 4 commits into from Aug 26, 2021
Merged

bootstrap-4 TextWidget wrappers now pull from registry, add rootSchema to Registry, Fix FieldProps.onFocus type to match WidgetProps #2519

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f7c81d
In all the bootstrap-4 `TextWidget` wrappers, rather than directly im…
heath-freenome Aug 17, 2021
c6c7df6
Merge branch 'master' into bootstrap-4-pulls-TextWidget-from-registry
heath-freenome Aug 17, 2021
8cf72b8
Update index.d.ts
heath-freenome Aug 17, 2021
d1379b8
Update index.d.ts
heath-freenome Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/bootstrap-4/src/ArrayFieldTemplate/ArrayFieldTemplate.tsx
View file
Open in desktop
Expand Up @@ -13,8 +13,7 @@ const { isMultiSelect, getDefaultRegistry } = utils;
const ArrayFieldTemplate = (props: ArrayFieldTemplateProps) => {
const { schema, registry = getDefaultRegistry() } = props;

// TODO: update types so we don't have to cast registry as any
if (isMultiSelect(schema, (registry as any).rootSchema)) {
if (isMultiSelect(schema, registry.rootSchema)) {
return <DefaultFixedArrayFieldTemplate {...props} />;
} else {
return <DefaultNormalArrayFieldTemplate {...props} />;
Expand Down
6 changes: 4 additions & 2 deletions packages/bootstrap-4/src/ColorWidget/ColorWidget.tsx
View file
Open in desktop
@@ -1,7 +1,9 @@
import React from "react";
import TextWidget, { TextWidgetProps } from "../TextWidget";
import { WidgetProps } from '@rjsf/core';

const ColorWidget = (props: TextWidgetProps) => {
const ColorWidget = (props: WidgetProps) => {
const { registry } = props;
const { TextWidget } = registry.widgets;
return <TextWidget {...props} type="color" />;
};

Expand Down
9 changes: 5 additions & 4 deletions packages/bootstrap-4/src/DateTimeWidget/DateTimeWidget.tsx
View file
Open in desktop
@@ -1,17 +1,18 @@
import React from "react";
import { utils } from "@rjsf/core";
import TextWidget, { TextWidgetProps } from "../TextWidget";
import { utils, WidgetProps } from "@rjsf/core";

const { localToUTC, utcToLocal } = utils;

const DateTimeWidget = (props: TextWidgetProps) => {
const DateTimeWidget = (props: WidgetProps) => {
const { registry } = props;
const { TextWidget } = registry.widgets;
const value = utcToLocal(props.value);
const onChange = (value: any) => {
props.onChange(localToUTC(value));
};

return (
<TextWidget
<TextWidget
{...props}
type="datetime-local"
value={value}
Expand Down
10 changes: 6 additions & 4 deletions packages/bootstrap-4/src/DateWidget/DateWidget.tsx
View file
Open in desktop
@@ -1,13 +1,15 @@
import React from "react";
import TextWidget, { TextWidgetProps } from "../TextWidget";
import { WidgetProps } from '@rjsf/core';

const DateWidget = (props: TextWidgetProps) => {
const DateWidget = (props: WidgetProps) => {
const { registry } = props;
const { TextWidget } = registry.widgets;
return (
<TextWidget
{...props}
{...props}
type="date"
/>
);
};

export default DateWidget;
export default DateWidget;
6 changes: 4 additions & 2 deletions packages/bootstrap-4/src/EmailWidget/EmailWidget.tsx
View file
Open in desktop
@@ -1,7 +1,9 @@
import React from "react";
import TextWidget, { TextWidgetProps } from "../TextWidget";
import { WidgetProps } from '@rjsf/core';

const EmailWidget = (props: TextWidgetProps) => {
const EmailWidget = (props: WidgetProps) => {
const { registry } = props;
const { TextWidget } = registry.widgets;
return <TextWidget {...props} type="email" />;
};

Expand Down
6 changes: 4 additions & 2 deletions packages/bootstrap-4/src/FileWidget/FileWidget.tsx
View file
Open in desktop
@@ -1,7 +1,9 @@
import React from "react";
import TextWidget, { TextWidgetProps } from "../TextWidget";
import { WidgetProps } from '@rjsf/core';

const FileWidget = (props: TextWidgetProps) => {
const FileWidget = (props: WidgetProps) => {
const { registry } = props;
const { TextWidget } = registry.widgets;
return <TextWidget {...props} type="file"/>;
};

Expand Down
8 changes: 2 additions & 6 deletions packages/bootstrap-4/src/TextWidget/TextWidget.tsx
View file
Open in desktop
Expand Up @@ -4,10 +4,6 @@ import Form from "react-bootstrap/Form";

import { WidgetProps } from "@rjsf/core";

export interface TextWidgetProps extends WidgetProps {
type?: string;
}

const TextWidget = ({
id,
placeholder,
Expand All @@ -25,7 +21,7 @@ const TextWidget = ({
schema,
rawErrors = [],

}: TextWidgetProps) => {
}: WidgetProps) => {
const _onChange = ({
target: { value },
}: React.ChangeEvent<HTMLInputElement>) =>
Expand All @@ -36,7 +32,7 @@ const TextWidget = ({
target: { value },
}: React.FocusEvent<HTMLInputElement>) => onFocus(id, value);
const inputType = (type || schema.type) === 'string' ? 'text' : `${type || schema.type}`
Copy link
Member

Choose a reason for hiding this comment

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

The one thing remaining is that I'm a bit confused as to why TextWidget uses the type prop, but only in the bootstrap-4 theme. Could you point me to the code that actually passes the type prop? And would it be better / equivalent / more consistent to change this line of code to not use type, in favor of something like this (https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/fluent-ui/src/TextWidget/TextWidget.tsx#L73)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type prop is also used in the @material-ui theme... but the TextWidgetProps there have been expanded to include the @material-ui TextField props, so passing type in works there (sadly I spent a decent amount of time trying to apply a similar solution to bootstrap-4 with no success :( ). See https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/material-ui/src/TextWidget/TextWidget.tsx#L11. I'm not against doing what fluent-ui does with type for bootstrap-4 if that feels more comfortable for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am curious if you feel like the WidgetProps should be restricted to only those current props defined on it, unlike the FieldProps which has the catch-all "other props" [prop: string]: any; defined on it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, with the change to WidgetProps that I'm proposing, I could probably remove the override that material-ui has since the catch-all would do the same thing. Heck, most of the code between the three packages for the TextWidget overrides could probably be standardized into the same exact code (most of which very closely matches how core does it)... which makes me wonder whether it makes sense to do a bit more work in core to use TextWidget rather than BaseInput... or make all three Typescript implementations simple provide their TextWidget implementation as the BaseInput and remove all the overrides that are simply the override the "type" since thats what core already does.

Copy link
Member

Choose a reason for hiding this comment

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

I am curious if you feel like the WidgetProps should be restricted to only those current props defined on it, unlike the FieldProps which has the catch-all "other props" [prop: string]: any; defined on it?

I think using the catch-all is OK for this PR.

Honestly, with the change to WidgetProps that I'm proposing, I could probably remove the override that material-ui has since the catch-all would do the same thing. Heck, most of the code between the three packages for the TextWidget overrides could probably be standardized into the same exact code (most of which very closely matches how core does it)... which makes me wonder whether it makes sense to do a bit more work in core to use TextWidget rather than BaseInput... or make all three Typescript implementations simple provide their TextWidget implementation as the BaseInput and remove all the overrides that are simply the override the "type" since thats what core already does.

Yeah, if you'd like to do additional refactoring, that would also be good. Let's not do it in this PR, though, maybe in a separate PR if you are interested in doing it.


// const classNames = [rawErrors.length > 0 ? "is-invalid" : "", type === 'file' ? 'custom-file-label': ""]
return (
<Form.Group className="mb-0">
Expand Down
6 changes: 4 additions & 2 deletions packages/bootstrap-4/src/URLWidget/URLWidget.tsx
View file
Open in desktop
@@ -1,7 +1,9 @@
import React from "react";
import TextWidget, { TextWidgetProps } from "../TextWidget";
import { WidgetProps } from '@rjsf/core';

const URLWidget = (props: TextWidgetProps) => {
const URLWidget = (props: WidgetProps) => {
const { registry } = props;
const { TextWidget } = registry.widgets;
return <TextWidget {...props} type="url" />;
Copy link
Member Author

Choose a reason for hiding this comment

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

@epicfaace This is one place where type is passed... material-ui uses the same exact pattern. fluent-ui chose to put type into the ui:options.props and then extracts it on the line above what you mentioned, filters out non-fluent props and spreads it over the default type that is being extracted on the line you mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, this makes sense now

};

Expand Down
5 changes: 4 additions & 1 deletion packages/bootstrap-4/test/helpers/createMocks.ts
View file
Open in desktop
@@ -1,6 +1,8 @@
import { WidgetProps } from "@rjsf/core";
import { JSONSchema7 } from "json-schema";

import TextWidget from "../../src/TextWidget/TextWidget";

export const mockSchema: JSONSchema7 = {
type: "array",
items: {
Expand Down Expand Up @@ -33,9 +35,10 @@ export function makeWidgetMockProps(
placeholder: "",
registry: {
fields: {},
widgets: {},
widgets: { TextWidget },
formContext: {},
definitions: {},
rootSchema: {},
},
...props,
};
Expand Down
2 changes: 2 additions & 0 deletions packages/core/index.d.ts
View file
Open in desktop
Expand Up @@ -115,6 +115,7 @@ declare module '@rjsf/core' {
multiple: boolean;
rawErrors: string[];
registry: Registry;
type?: string; // Add the optional prop for TextWidget to simplify Typescript usage
epicfaace marked this conversation as resolved.
Show resolved Hide resolved
}

export type Widget = React.StatelessComponent<WidgetProps> | React.ComponentClass<WidgetProps>;
Expand All @@ -124,6 +125,7 @@ declare module '@rjsf/core' {
widgets: { [name: string]: Widget };
definitions: { [name: string]: any };
formContext: any;
rootSchema: JSONSchema7;
}

export interface FieldProps<T = any>
Expand Down
3 changes: 1 addition & 2 deletions packages/fluent-ui/src/ArrayFieldTemplate/ArrayFieldTemplate.tsx
View file
Open in desktop
Expand Up @@ -16,8 +16,7 @@ const { isMultiSelect, getDefaultRegistry } = utils;
const ArrayFieldTemplate = (props: ArrayFieldTemplateProps) => {
const { schema, registry = getDefaultRegistry() } = props;

// TODO: update types so we don't have to cast registry as any
if (isMultiSelect(schema, (registry as any).rootSchema)) {
if (isMultiSelect(schema, registry.rootSchema)) {
return <DefaultFixedArrayFieldTemplate {...props} />;
} else {
return <DefaultNormalArrayFieldTemplate {...props} />;
Expand Down
3 changes: 1 addition & 2 deletions packages/material-ui/src/ArrayFieldTemplate/ArrayFieldTemplate.tsx
View file
Open in desktop
Expand Up @@ -19,8 +19,7 @@ const {
const ArrayFieldTemplate = (props: ArrayFieldTemplateProps) => {
const { schema, registry = getDefaultRegistry() } = props;

// TODO: update types so we don't have to cast registry as any
if (isMultiSelect(schema, (registry as any).rootSchema)) {
if (isMultiSelect(schema, registry.rootSchema)) {
return <DefaultFixedArrayFieldTemplate {...props} />;
} else {
return <DefaultNormalArrayFieldTemplate {...props} />;
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/test/UpDownWidget.test.tsx
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@ describe("UpDownWidget", () => {
widgets: {},
definitions: {},
formContext: {},
rootSchema: {},
}
};
const tree = renderer.create(<UpDownWidget {...props} />).toJSON();
Expand Down