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

heath-freenome · 2021-08-17T19:05:36Z

Reasons for making this change

These changes allow all of these TextWidget wrappers (like EmailWidget, etc...) to immediately use a custom TextWidget class
Updated the Registry type in the core/index.d.ts file to add rootSchema and fix the indenting
- This fixed a TODO and avoids the need to cast registry to any in the three typescript ArrayFieldTemplate implementations
- Replaced the usage FieldProps['registry'] with Registry now that a separate type exists
Also updated WidgetProps to add a generic [prop: string]: any just like FieldProps has
- This eliminated the custom TextWidgetProps for bootstrap-4 which prevented a previous PR from converting the TextWidget wrappers
Finally, made the FieldProps.onFocus match the definition of WidgetProps.onFocus to avoid type conversion error when creating custom Field components that pass onFocus to a Widget
Updated all of the bootstrap-4 widgets to use TextWidget from the registry, using simple WidgetProps
- Updated TextWidget to remove TextWidgetProps and to simply use WidgetProps
Updated the material-ui UpDownWidget tests to add a blank rootSchema to the registry
Updated the bootstrap-4 createMock() to add a blank rootSchema to the registry as well as adding TextWidget to the registry.widgets

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

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

…port `TextWidget` from the source hierarchy, instead pull from the `registry.widgets` - These changes allow all of these `TextWidget` wrappers (like `EmailWidget`, etc...) to immediately use a custom `TextWidget` class - Updated the `Registry` type in the `core/index.d.ts` file to add `rootSchema` - This fixed a TODO and avoids the need to cast `registry` to any in the three typescript `ArrayFieldTemplate` implementations - Also updated `WidgetProps` to add an optional `type` - This eliminated the custom `TextWidgetProps` for `bootstrap-4` which prevented a previous PR from converting the `TextWidget` wrappers - Updated all of the `bootstrap-4` widgets to use `TextWidget` from the registry, using simple `WidgetProps` - Updated `TextWidget` to remove `TextWidgetProps` and to simply use `WidgetProps` - Updated the `material-ui` `UpDownWidget` tests to add a blank `rootSchema` to the `registry` - Updated the `bootstrap-4` `createMock()` to add a blank `rootSchema` to the `registry` as well as adding `TextWidget` to the `registry.widgets`

heath-freenome · 2021-08-17T19:07:37Z

@epicfaace I figured out how to fix bootstrap-4 for #2512 assuming the change I made to WidgetProps is acceptable.

packages/core/index.d.ts

- Made the `FieldProps.onFocus` match the definition of `WidgetProps.onFocus` to avoid type conversion error when creating custom components

heath-freenome · 2021-08-19T16:37:01Z

@epicfaace I imagine you are rather busy... Is there another person who you know of who can complete this review?

epicfaace · 2021-08-19T19:17:36Z

Maybe @jimmycallin?

…

-- Ashwin Ramaswami

On Thu, Aug 19, 2021 at 12:37 PM Heath C ***@***.***> wrote: @epicfaace <https://github.com/epicfaace> I imagine you are rather busy... Is there another person who you know of who can complete this review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2519 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4MX4EBVJDKMUSQ3KGZE3T5UXLPANCNFSM5CKPPZWA> .

heath-freenome · 2021-08-23T21:52:02Z

@epicfaace or @jimmycallin I'd love to be able to merge these changes. I hope one of you two can complete the review soon

heath-freenome · 2021-08-25T22:40:39Z

@epicfaace Is there one or two other PRs that I could review for you in return for a review on this? Are you worried about the type changes causing problems?

epicfaace · 2021-08-25T23:14:26Z

packages/bootstrap-4/src/TextWidget/TextWidget.tsx

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


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)?

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.

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?

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.

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.

heath-freenome · 2021-08-26T16:26:37Z

packages/bootstrap-4/src/URLWidget/URLWidget.tsx

-const URLWidget = (props: TextWidgetProps) => {
+const URLWidget = (props: WidgetProps) => {
+  const { registry } = props;
+  const { TextWidget } = registry.widgets;
  return <TextWidget {...props} type="url" />;


@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.

Thanks, this makes sense now

epicfaace · 2021-08-26T20:36:04Z

packages/bootstrap-4/src/URLWidget/URLWidget.tsx

-const URLWidget = (props: TextWidgetProps) => {
+const URLWidget = (props: WidgetProps) => {
+  const { registry } = props;
+  const { TextWidget } = registry.widgets;
  return <TextWidget {...props} type="url" />;


Thanks, this makes sense now

epicfaace · 2021-08-26T20:37:04Z

packages/bootstrap-4/src/TextWidget/TextWidget.tsx

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


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.

heath-freenome · 2021-08-26T21:14:38Z

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.

I am interested in doing it... Do I need an issue for that or just push a PR when it's ready?

epicfaace · 2021-08-27T01:46:34Z

That'd be great! An issue is not required, but it might be nice to make an issue if it doesn't take too much time so it's easier to organize what work is going to be done.

Merge branch 'master' into bootstrap-4-pulls-TextWidget-from-registry

c6c7df6

epicfaace reviewed Aug 17, 2021

View reviewed changes

packages/core/index.d.ts Outdated Show resolved Hide resolved

Update index.d.ts

8cf72b8

- Made the `FieldProps.onFocus` match the definition of `WidgetProps.onFocus` to avoid type conversion error when creating custom components

heath-freenome changed the title ~~bootstrap-4 TextWidget wrappers now pull from registry, add rootSchema to Registry~~ bootstrap-4 TextWidget wrappers now pull from registry, add rootSchema to Registry, Fix FieldProps.onFocus type to match WidgetProps Aug 17, 2021

heath-freenome requested a review from epicfaace August 18, 2021 17:30

Update index.d.ts

d1379b8

epicfaace reviewed Aug 25, 2021

View reviewed changes

heath-freenome commented Aug 26, 2021

View reviewed changes

epicfaace approved these changes Aug 26, 2021

View reviewed changes

epicfaace merged commit 9c0dd42 into rjsf-team:master Aug 26, 2021

heath-freenome deleted the bootstrap-4-pulls-TextWidget-from-registry branch February 22, 2022 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

heath-freenome commented Aug 17, 2021 •

edited

Loading

heath-freenome commented Aug 17, 2021

heath-freenome commented Aug 19, 2021

epicfaace commented Aug 19, 2021 via email

heath-freenome commented Aug 23, 2021

heath-freenome commented Aug 25, 2021

epicfaace Aug 25, 2021

heath-freenome Aug 25, 2021 •

edited

Loading

heath-freenome Aug 26, 2021 •

edited

Loading

heath-freenome Aug 26, 2021

epicfaace Aug 26, 2021

heath-freenome Aug 26, 2021

epicfaace Aug 26, 2021

epicfaace Aug 26, 2021

epicfaace Aug 26, 2021

heath-freenome commented Aug 26, 2021

epicfaace commented Aug 27, 2021

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

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

Conversation

heath-freenome commented Aug 17, 2021 • edited Loading

Reasons for making this change

Checklist

heath-freenome commented Aug 17, 2021

heath-freenome commented Aug 19, 2021

epicfaace commented Aug 19, 2021 via email

heath-freenome commented Aug 23, 2021

heath-freenome commented Aug 25, 2021

epicfaace Aug 25, 2021

Choose a reason for hiding this comment

heath-freenome Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

heath-freenome Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

heath-freenome Aug 26, 2021

Choose a reason for hiding this comment

epicfaace Aug 26, 2021

Choose a reason for hiding this comment

heath-freenome Aug 26, 2021

Choose a reason for hiding this comment

epicfaace Aug 26, 2021

Choose a reason for hiding this comment

epicfaace Aug 26, 2021

Choose a reason for hiding this comment

epicfaace Aug 26, 2021

Choose a reason for hiding this comment

heath-freenome commented Aug 26, 2021

epicfaace commented Aug 27, 2021

heath-freenome commented Aug 17, 2021 •

edited

Loading

heath-freenome Aug 25, 2021 •

edited

Loading

heath-freenome Aug 26, 2021 •

edited

Loading