Make material-ui and fluent-ui pull TextWidget from the registry; remove registry prop from <div> in TextWidget #2515
Conversation
- In all the `TextWidget` wrappers, rather than directly import `TextWidget` from the source hierarchy, instead pull from the `registry.widgets` - This allows all of these `TextWidget` wrappers (like `EmailWidget`, etc...) to immediately use a custom `TextWidget` class - Added a new `Registry` type in the `core/index.d.ts` file, refactoring it from `FieldProps`, and adding it to `WidgetProps` as well - Needed to updated the snapshots for the `material-ui` tests for `Form` since registry is now part of the snapshot - Also needed to update `UpDownWidget.test.tst` to add a mock `registry` - Updated `packages/bootstrap-4/test/helpers/createMocks.ts` to add a mock `registry`
|
@epicfaace Here's the PR... let me know what is needed to get it merged |
| const PasswordWidget = (props: TextWidgetProps) => { | ||
| const { registry } = props; | ||
| const { TextWidget } = registry.widgets; | ||
| return <TextWidget type="password" {...props} />; |
There was a problem hiding this comment.
Will this render the exact same content as the old PasswordWidget component? Trying to see if this will be a breaking change or not.
There was a problem hiding this comment.
The snapshot update was to add the registry to the outer div (which seems weird and probably should be removed). The only other thing was adding an empty placeholder to the input, which honestly might have been a bug with the PasswordWidget before.
There was a problem hiding this comment.
@epicfaace let me know if stripping the registry from the outer div (as seen in the snapshots) is considered a breaking change. Honestly, to me it feels like a super helpful fix since the whole registry object being dumped to the div was just useless clutter on the div tag... especially when the rootSchema is HUGE
There was a problem hiding this comment.
Yeah, I think it should be fine, not considered a breaking change since that doesn't affect how users use the library / any of the look-and-feel at all.
…ome part of `textFieldProps` which just seems wrong
epicfaace
changed the title
epicfaace
changed the title
|
Yay! Thanks for the approval! I look forward to updating my code to use these changes |
heath-freenome
deleted the
TextWidget-wrappers-use-registry-material-and-fluent-ui
branch
Reasons for making this change
In all the
TextWidgetwrappers, rather than directly importTextWidgetfrom the source hierarchy, instead pull from theregistry.widgetsTextWidgetwrappers (likeEmailWidget, etc...) to immediately use a customTextWidgetclassRegistrytype in thecore/index.d.tsfile, refactoring it fromFieldProps, and adding it toWidgetPropsas wellmaterial-uiandfluent-uiwidgets to useTextWidgetfrom the registrymaterial-uiPasswordWidgetto simply useTextWidgetin a manner similar the othermaterial-uiwrappersfluent-uisimply wrapsTextWidgetand it seemed like doing so here made sense as wellTextWidgetto declareregistryin the props so it doesn't get collected into thetextFieldPropsmaterial-uitests sinceregistryis no longer part of the snapshotsplaceholder=''to thePasswordWidget, since password inputs are allowed to have them in html 5 it seems like a small bug fixUpDownWidget.test.tsxto add a mockregistrypackages/bootstrap-4/test/helpers/createMocks.tsto add a mockregistryfixes #2512
Checklist