Skip to content

Make BaseInput overridable#699

Merged
glasserc merged 3 commits intorjsf-team:masterfrom
olzraiti:overridableBaseInput
Sep 29, 2017
Merged

Make BaseInput overridable#699
glasserc merged 3 commits intorjsf-team:masterfrom
olzraiti:overridableBaseInput

Conversation

@olzraiti
Copy link
Copy Markdown
Contributor

@olzraiti olzraiti commented Sep 15, 2017

Reasons for making this change

All the other components in registry fields & widgets are overridable. BaseInput shouldn't be an exception. Customizing BaseInput is useful e.g. for propagating changes only on text input blur.

BTW there is something wrong with the npm commit hook - after pushing the first commit I noticed that some files were prettified after pushing, but the prettified changes were in git staging and weren't pushed.

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@glasserc
Copy link
Copy Markdown
Contributor

Looks OK to me. Can we add a brief note to the documentation explaining that this is possible and why you might want to do this?

Regarding the commit hook, I've never heard of it accidentally not committing stuff. I'm more familiar with husky committing too much stuff, as in Kinto/kinto-admin#419.

@Chun-Yang
Copy link
Copy Markdown

Can we change 'BaseInput' to 'BaseInputWidget' to make it consistent?
Since BaseInput is not an open API, the name change should not affect anyone anyway.

@glasserc
Copy link
Copy Markdown
Contributor

That sounds OK to me.

@olzraiti
Copy link
Copy Markdown
Contributor Author

Can we change 'BaseInput' to 'BaseInputWidget' to make it consistent?
Since BaseInput is not an open API, the name change should not affect anyone anyway.

On the other hand, excluding the "Widget" part in the component/file name communicates that it's not a widget. It isn't a widget because you can't use BaseInput with ui:widget in uiSchema. Maybe we should move it away from widgets directory or use an underscore in the file name, but I think it's OK like it is now.

@glasserc glasserc merged commit 3c070ee into rjsf-team:master Sep 29, 2017
@glasserc
Copy link
Copy Markdown
Contributor

Thanks!

@anlesk
Copy link
Copy Markdown
Contributor

anlesk commented Nov 20, 2017

Hi @olzraiti @glasserc @n1k0,

I've just updated to the latest version of react-jsonschema-form and faced the issue with Jest snapshot tests. We have a custom widget that is actually a wrapper for original TextWidget.
With that PR being merged to master, snapshot test of custom widget is starting to fail with "unable to read property widgets of undefined".

After some investigation, I found out that the reason is that line:
const { BaseInput } = props.registry.widgets;.

Shouldn't default registry be defined as a default prop for TextWidget? I found a workaround providing registry as a property for custom widget but that looks like a hack to me.

import { getDefaultRegistry } from 'react-jsonschema-form/lib/utils';

// in render
<CustomTextWidget  registry={getDefaultRegistry()} />

Please share your thoughts.

@olzraiti
Copy link
Copy Markdown
Contributor Author

The registry property must be passed to child fields/widgets always. If BaseInput doesn't receive registry, then it's likely that some of your custom fields/widgets doesn't pass the registry property properly.

We can't use the default registry as a default, because then the custom fields/widgets wouldn't be used.

@anlesk
Copy link
Copy Markdown
Contributor

anlesk commented Nov 24, 2017

@olzraiti, thanks for the response!

Shouldn't we define registry as a required propType than?

@olzraiti
Copy link
Copy Markdown
Contributor Author

@anlesk Yes, good idea. Feel free to open a pull request for that :)

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.

4 participants