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

Upgrade Material Renderer Inputs to be aware of Variant #1797

Closed
crgmz opened this issue Aug 15, 2021 · 13 comments · Fixed by #2182 or #2256
Closed

Upgrade Material Renderer Inputs to be aware of Variant #1797

crgmz opened this issue Aug 15, 2021 · 13 comments · Fixed by #2182 or #2256

Comments

@crgmz
Copy link

crgmz commented Aug 15, 2021

Is your feature request related to a problem? Please describe.

When using the material renderers, it would be nice to have the option to use the different material-ui input variants (standard, filled, outlined).

Currently the material renderers are implemented using the Input component with no option to change the variant. That means you have to implement custom renderers if you want to use other variants.

Describe the solution you'd like

The ideal solution would be updating the material renderers to take into consideration variants declared in a custom material theme provided to the JsonForms component, that way you can use material-ui's built-in theme support to build a custom theme and material renderers would choose the appropriate variant (or the standard variant if no custom theme is defined).

Describe alternatives you've considered

There's two alternatives by implementing custom renderers:

  1. Copy/pasting the material renderers source and changing the Input component to the variant you want. This means you now how to keep track of changes to source for this renderer.
  2. Implementing your own custom renderer from scratch, meaning you have to re-invent the wheel and take care of possible edge cases that the material renderers library already covers.

Framework

React

RendererSet

Material

Additional context

If the team agrees this would be a good feature to implement, I'm willing to work on it and submit a PR.

@sdirix
Copy link
Member

sdirix commented Aug 23, 2021

Hi @CarlosGomez-dev! Thanks for the feature request.

In general this definitely makes sense, so if we could add this feature in a non-disruptive way then I'm definitely for it.

Can you give a small example of what you're suggesting? If I understand correctly you would like to suggest using Input, FilledInput and OutlinedInput depending on whether the user customized the props of the corresponding Material UI Form Input? So for example when a user set the variant of TextField to filled we need to check all textual inputs and render FilledInput then?

It's a little bit misleading in a way that we are not actually using TextFields so the maybe this maps sometimes in unexpected ways. Or are you suggesting that we introduce custom JSON Forms options? So for example jsonforms.input.variant could then just set the variant for all inputs. This would be similar to the existing custom theming support.

Did you already implement an approach for your own renderer set?

@crgmz
Copy link
Author

crgmz commented Aug 25, 2021

Hello @sdirix,

Can you give a small example of what you're suggesting? If I understand correctly you would like to suggest using Input, FilledInput and OutlinedInput depending on whether the user customized the props of the corresponding Material UI Form Input? So for example when a user set the variant of TextField to filled we need to check all textual inputs and render FilledInput then? It's a little bit misleading in a way that we are not actually using TextFields so the maybe this maps sometimes in unexpected ways.

That is what I had in mind, default props is material-ui calls them.

I took a deeper look and can see that MaterialInputControl is rendering textual inputs inside a FormControl, so you would need to target MuiFormControl instead of MuiTextField in a custom theme, but users would need to dive into the source code to learn that which is less than ideal.

Or are you suggesting that we introduce custom JSON Forms options? So for example jsonforms.input.variant could then just set the variant for all inputs. This would be similar to the existing custom theming support.

Seeing the discoverability issue above, this approach seems more user-friendly. This means MaterialInputControl would need to be aware of this property, as the label variant must match the input variant, and then every material-renderer input also needs to be aware of this property to render the correct variant.

I would implement this similarly to how the official TextInput handles it.

Did you already implement an approach for your own renderer set?

I haven't. I created a custom renderer set with the specific variant and style I needed, but a feature like this would be very useful as I could simply define a variant via jsonforms.input.variant and if I wanted to define a custom theme I would know exactly which input to target. (MuiInput/MuiFilledInput/MuiOutlinedInput)

@sdirix
Copy link
Member

sdirix commented Aug 30, 2021

Hi @CarlosGomez-dev, sounds good to me! I'll discuss with the team and come back to you next week.

@sdirix
Copy link
Member

sdirix commented Sep 9, 2021

Hi @CarlosGomez-dev, we very much would like to see this feature. So if you have time a PR is very welcomed!

Regarding the implementation we're wondering whether it should be implemented by determining the appropriate Input variant component in MaterialInputControl and then hand it over the to InnerComponent. This way the determination code does not have to be duplicated in all MuiInputXYZ components. What do you think?

@ogizanagi
Copy link

I would love this ❤️ . Are you still working on this @CarlosGomez-dev ?

@klemenoslaj
Copy link

I just bumped in this issue as well. Naively I assumed simply adjusting the default properties in MUI theme would do the trick.

I saw however that JSONForms uses lower level components, like Input instead of TextField.
I just quickly ran trough the code, changed all variants to outline and changed Input to TextField and that resolved all my issues.

I however do not have any insight why the lower level components were used.

@sdirix is there some specific reason for that?

I have three options:

  • contribute refactoring to properly apply mui theming
  • fork and maintain my own material controls
  • use alternative library or custom implementation

I would prefer contributing but only if that is in alignment with the project.

@sdirix
Copy link
Member

sdirix commented Jul 25, 2022

Hi! The use of lower level inputs has historical reasons. Originally the cells renderers were mandatory and not only used in Tables but reused in all control renderers. At some point this was refactored, so now we have dedicated self-contained renderers, however internally they still use the cells primitives. Just no longer by dispatching to them but by reusing an extracted component.

We are definitely willing to accept changes here. Feel free to contribute using the approach outlined above

In case you want to go the full way and replace all invocations of MaterialInputControl with the fully integrated variant of Material UI, e.g. TextField, then I would recommend first doing an analysis of the consequences so we can internally discuss whether that is the way we want to go. For this you could for example replace only the MaterialTextControl and MaterialNumberControl and highlight the required code changes.

Note that we require test cases for all contributions, especially ones which change a lot of the render output ;)

@klemenoslaj
Copy link

I see in the approach outlined above you guys discussed a custom config to the JSONForms directly, right?
But why would that be better than relying on the MUI theming configuration?

Effectively a custom TextField is created in JSONForms library, so I guess using the MuiTextField in theming config would be completely fine and expected.


So what I could do is reverse engineer the TextField without introducing breaking changes to JSONForms and switch between different variants of inputs.
I would do most of the work in MaterialInputControl to avoid duplications and pass down the correct variant.

Something like:

const { ..., variant = 'standard' } = useThemeProps({
    props: props as ControlProps & WithInput & TextFieldProps,
    name: 'MuiTextField',
});

const variantComponent = {
  standard: Input,
  filled: FilledInput,
  outlined: OutlinedInput,
};
const VariantComponent = variantComponent[variant];

...

return (
  <FormControl
    ...
    variant={variant}
  >
    <InnerComponent
      {...props}
      id={id + '-input'}
      isValid={isValid}
      visible={visible}
      VariantComponent={VariantComponent}
    />
  </FormControl>
)

@stumpykilo
Copy link

@klemenoslaj I started on this today but would really like more insight into what you were thinking. I'm pretty new to JSONForms and I'm not quite sure how you would use VariantComponent as a property to the InnerComponent. Any elaboration would be greatly appreciated.

@zekenie
Copy link

zekenie commented Jun 2, 2023

Agreed, this tool looks wonderful, and I don't think we can use it without the ability to configure MUI a little bit more

@sebastianfrey
Copy link
Contributor

@sdirix Is someone working on this issue? If not, I am willing to provide a PR which would leverage MUI's theme provider setting for variant.

@sdirix
Copy link
Member

sdirix commented Sep 29, 2023

Hi @sebastianfrey, so you would do the work in MaterialInputControl as suggested above? Is yes, then that's fine!

@sebastianfrey
Copy link
Contributor

@sdirix, yes, as suggested in the linked comment I would leverage useThemeProps() from MUI to implement the desired behavior.

lucas-koehler pushed a commit that referenced this issue Oct 9, 2023
Extend the react material renderers to be aware of MUI's input variant in the theme.

- Extend input controls to be aware of the configured variant and use it
- Extend input renderers to set the variant on the form control
- Extend react material example with a variant selection dropdown

Fix #1797
@lucas-koehler lucas-koehler modified the milestones: next, 3.2 Oct 9, 2023
@sdirix sdirix linked a pull request Jan 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment