mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
94.09k stars 32.33k forks source link

[FormControl] Cannot render `FormControl` as `React.Fragment` #38094

Open thany opened 1 year ago

thany commented 1 year ago

Duplicates

Latest version

Steps to reproduce πŸ•Ή

Link to live example: https://codesandbox.io/s/zealous-benz-3zmg7x?file=/src/App.tsx

Steps:

  1. Use a FormControl component anywhere
  2. Make it render as a Fragment
  3. See error in the console.

Current behavior 😯

It produces an error in the console:

Warning: Invalid prop `className` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.

Expected behavior πŸ€”

No error.

Context πŸ”¦

I'm doing this because I need FormControl not to produce extra elements around my form elements, because of CSS Grid.

Applying display: contents is not sufficient for a CSS Grid to work, and subgrid is sadly still not widely enough supported.

Your environment 🌎

npx @mui/envinfo ``` System: OS: Windows 10 10.0.19045 Binaries: Node: 18.15.0 - C:\Program Files\nodejs\node.EXE Yarn: Not Found npm: 9.8.0 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: 115.0.5790.102 Edge: Spartan (44.19041.1266.0), Chromium (114.0.1823.86) npmPackages: @emotion/react: 11.10.6 @emotion/styled: 11.10.6 @mui/base: 5.0.0-alpha.124 @mui/core-downloads-tracker: 5.11.16 @mui/material: 5.11.16 @mui/private-theming: 5.12.0 @mui/styled-engine: 5.11.16 @mui/styles: 5.12.0 @mui/system: 5.11.16 @mui/types: 7.2.4 @mui/utils: 5.12.0 @types/react: 18.2.0 react: 18.2.0 react-dom: 18.2.0 typescript: 4.9.5 ```

I'm using Firefox 115, just in case it matters.

thany commented 1 year ago

One possible (but silly) workaround:

const Noop: React.FC<React.PropsWithChildren> = ({ children }) => (
  <>{children}</>
);

const MyAwesomeFormLine: React.FC = () => {
  <FormControl component={Noop}>
    // Label + Input
  </FormControl>
};

So basically Noop in place of Fragment where Noop eats away the error the Fragment would normally spew out to warn about props getting passed. Not very elegant, iyam, but at least it cleans up the console.

mj12albert commented 1 year ago

@thany I'm pretty sure this cannot work as FormControl needs to receive props for it's intended functionality, but a Fragment cannot receive anything other than key and children as the error message explains

I'm doing this because I need FormControl not to produce extra elements around my form elements, because of CSS Grid.

Can you share more about your use-case or the layout you are trying to achieve?

thany commented 1 year ago

Each FormControl contains a label and an input control. I (we & our designer) despise the abel-inside-or-above-the-control layout, and we have to deliver a classic but proven layout, labels on the left, fields on the right.

This is impossible to achieve if each FormControl produces a <div>, because for CSS Grid to work, all cells are required to be its children.

Currently, having multiple FormControl components for a form, produces roughly this (pseudo code):

<form>
  <div> <!-- FormControl -->
    <label>
    <input>
  <div> <!-- FormControl -->
    <label>
    <input>
  <div> <!-- FormControl -->
    <label>
    <input>

Obne cannot apply CSS Grid on this and expect the labels and inputs to magically become the grid cells. CSS Grid just simply doesn't work like that. The only correct way to get this to render correct as a grid, is to eliminate those divs:

<form>
  <> <!-- FormControl -->
    <label>
    <input>
  <> <!-- FormControl -->
    <label>
    <input>
  <> <!-- FormControl -->
    <label>
    <input>

So that the labels and inputs become children of the form, which is the CSS Grid.

Now, to be brutally honest, I don't see why FormControl would absolutely need to render itself as a div. What for? The way I see it, FormControl is merely a empty component to provide some functionality to its contained form element components. It doesn't need to render anything itself. It current does (or tries to anyway πŸ˜’) but to what end? I don't need its classnames to exist in the HTML structure, and I certainly don't like to be limited to having each label+input combination wrapped in another element - I need and want the freedom not to require this.

To put this more generally - a UI framework should not dictate what HTML elements should be rendered outside its components. It should never render any HTML elements that only it needs, but the browser doesn't. Any wrapping functional-only component should not render any HTML, but should only provide the intended functionality to its child components. That's a lot of "should" so feel free to disagree, as long as we can solve the problem described above πŸ˜‰

Now, if there's any other component than FormControl I should be using instead, while still providing states and whatnot to its child input components, I'm all ears to hear about it πŸ™‚


Edit:

@thany I'm pretty sure this cannot work as FormControl needs to receive props for it's intended functionality, but a Fragment cannot receive anything other than key and children as the error message explains

Btw if I take that literally - FormControl can still take all the props in the world without rendering anything. Recieving props has nothing to do with whatever a component returns as. I'm sure you know this, but I just wanted to make sure we're on the same page.

The issue is that FormControl, being able to be told what component to render as, blindly assumes that everything it passes to that component, it can receive on its turn, which is clearly an unsafe assumption.

michaldudak commented 1 year ago

@mj12albert, I think we should be able to allow this. The FormControl doesn't set or rely on event handlers. We could either allow React.Fragment or null as component/slots.root or add a new prop that would control if the FormControlRoot is rendered at all.

thany commented 1 year ago

@mj12albert, I think we should be able to allow this. The FormControl doesn't set or rely on event handlers. We could either allow React.Fragment or null as component/slots.root or add a new prop that would control if the FormControlRoot is rendered at all.

I would rather like the latter, because it's entirely possible to build a component that also cannot take a className prop, or whatever else FormControl is trying to pass on. Add some strict propType validation and it too may spew out an error similar to the one from Fragment.