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.

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

make it more easy to replace components without requiring to implement a custom AutoField #640

Closed
macrozone opened this issue Nov 11, 2019 · 11 comments · Fixed by #800
Closed
Assignees
Labels
Type: Feature New features and feature requests
Milestone

Comments

@macrozone
Copy link
Contributor

If you just want to replace one field (e.g. replace the ListField with a custom one for all schemas), you need to reimplement the whole AutoField.

It would probably easier, if you could pass a getFieldComponent to a AutoForm, which would hook into here: https://github.com/vazco/uniforms/blob/master/packages/uniforms-material/src/AutoField.tsx#L24 and would take props and could return a Component. If no component is returned, it would fallback to the normal logic.

@kestarumper kestarumper self-assigned this Nov 13, 2019
@kestarumper kestarumper added the Type: Feature New features and feature requests label Nov 13, 2019
@radekmie radekmie assigned radekmie and unassigned kestarumper Nov 13, 2019
@radekmie
Copy link
Contributor

Hi, @macrozone. I'm somewhat against it, as I don't like the idea of modifying it by reference. The broader problem is that AutoField renders NestField (or ListField), which renders AutoField again. Due to this cycle, it's not obvious who should change. For now, a custom theme is a go-to choice for most cases; another one is to automatically set the component prop at the schema level for all fields.

In the future (v3), it'll be easier to extend the context (and typing it!), so maybe the AutoField (or the function to determine the component?) could be a part of the context data.

@radekmie radekmie mentioned this issue Nov 13, 2019
@macrozone
Copy link
Contributor Author

macrozone commented Nov 13, 2019

@radekmie i don't understand the reasoning at all, but let's see what v3 brings

Edit: its not modifying by reference, you just can create your extended AutoField which can render a different component on some field-types and pass that custom field to your AutoForm. Creating a custom theme for that is just overkill (at least with the current api). And since the AutoForm defines the AutoField anyway, you don't have that cycle problem.

@radekmie
Copy link
Contributor

You can easily create a custom AutoField right now - simply extend the current one and provide your custom render method, falling back to the super.render if needed. Passing it to the form is also possible. The problem is that if it'll render NestField, this NestField will render the default one, imported from the theme.

@macrozone
Copy link
Contributor Author

You can easily create a custom AutoField right now - simply extend the current one and provide your custom render method, falling back to the super.render if needed. Passing it to the form is also possible. The problem is that if it'll render NestField, this NestField will render the default one, imported from the theme.

ah yes, that's make it more clear. And in v3 the fields will come from the context?

@radekmie
Copy link
Contributor

Not sure if they will, but it'll be easier to do so, probably even in the project scope (no core change will be needed).

@Floriferous
Copy link
Contributor

We'd love something like this as well, we have pretty much rewritten all the uniforms-material components at this point, sometimes just to change one or two lines.

If there was a way to at least only rewrite components once, and then have them used by all recursive calls would be great.

Maybe an API like this could be helpful:

import { makeAutoField, FIELD_TYPES } from 'uniforms';

const AutoField = makeAutoField({
  getComponent: type => {
    if (type === FIELD_TYPES.NESTED) {
      return CustomListField;
    }
    if (type === FIELD_TYPES.SELECT) {
      // Could address #642 
      return (props) => <CustomSelectField {...props} options={[{ value: null, label: 'Pick one' }, ...props.options]} />;
    }
    if (type === FIELD_TYPES.RADIO) {
      // Merge these props in
      return { buttonColor: 'red' }
    }
    if (type === FIELD_TYPES.LIST) {
      // Allow adding stuff in the render method, keep it modular
      return { children: <><hr /><h2>This is a list field</h2></> }
    }
    // Return undefined to get the original included component
  }
})

@radekmie radekmie added this to the v3 milestone Jan 22, 2020
@radekmie
Copy link
Contributor

I've been thinking about it quite much lately. After experimenting with a few approaches, here are my thoughts:

  • Doing it externally (without touching the AutoField by simply creating a new one)...
    • ... requires passing the component in the context. It's not bad per se, but it unnecessary complicates it.
    • ... requires changes in composite fields like ListField or NestField. It's also hard to think out a way to make the built-in theme fields use your custom AutoField component.
  • Doing it internally (by changing the AutoField)...
    • ... complicates the AutoField by itself.
    • ... hides the complexity within one field. Everyone could import the AutoField and it'd be configurable from outside.
    • ... requires some way of configuration.

Currently, I think that making the AutoField aware of its uniqueness is the way to go. In this case, it'd mean to somehow keep this change to it, without touching anything else. I think we could use the React context for that:

// Option A
export default function AutoField(props: AutoFieldProps) {
  const AutoFieldComponent = useContext(AutoFieldContext);
  return <AutoFieldComponent {...props} />;
}

// Option B
export default function AutoField(props: AutoFieldProps) {
  const renderer = useContext(AutoFieldContext);
  return renderer(props);
}

(default implementation is set as a context default)

Upsides? Works out of the box, is 100% configurable and is backward compatible. Downsides? It creates an additional context. It shouldn't be a problem though, as it's probably not going to change often (if at all).

I'm just not sure if it's better to store an actual component in the context (option A) or a render function (option B). The former seems cleaner (simply provide a component), but the latter has one advantage - it creates no intermediate components (it'd look just like right now AutoField > CustomField).

@hmvp
Copy link

hmvp commented May 20, 2020

Somewhat related to this since I found myself in a similar position:
We generate the json schema in the backend and thus cannot set the component field directly, but we use a lot of custom components. Thus I found myself implementing my own version of AutoField. However I changed this, because I found that it was easier to add a getField method to my subclass of the bridge to set the component there based on type and format.

I still find it a bit weird that there are now two places which determine the correct component. Having an AutoField feels logical, but setting the correct component from the bridge based on bridge specific fields (type and format in case of json schema) also makes sense..

BTW: the current bridge design is a bit cumbersome for this since you need getField to do the above since getType only returns the field type which means you need to coordinate it with AutoField to actually use the custom component

@radekmie
Copy link
Contributor

Hi @hmvp. It's not really two places - it's the AutoField handled component prop. That's why the bridge could do it, but shouldn't (in general) - doing it couples UI with logic. The goal would be to make AutoField configurable enough not to touch non-UI code at all.

@wtrocki
Copy link

wtrocki commented May 26, 2020

We'd love something like this as well, we have pretty much rewritten all the uniforms-material components at this point, sometimes just to change one or two lines.

I can relate to this feedback. It is really easy to get some basic forms but when requirements changing it is hard to customize them without changing underlying UI packages.

@radekmie radekmie modified the milestones: v3.0, v3.1 Jun 6, 2020
@radekmie
Copy link
Contributor

radekmie commented Jun 6, 2020

I think I've found some middle ground that may work the best:

 export default function AutoField(originalProps: AutoFieldProps) {
   const props = useField(originalProps.name, originalProps)[0];
-  let { component } = props;
+  const detect = useContext(AutoFieldContext);
+  let { component = detect(props) } = props;

   // Nothing changes below.
 }

This means that the function that defines the component will receive all field props and acts only as a fallback (i.e. props > schema > detection). I think that leaving the current detection logic in is a good idea - otherwise one would have to use it directly in order to add one more rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants