mui / material-ui

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

[base][Tab] React does not recognize the `ownerState` prop #32882

Closed zzev closed 1 year ago

zzev commented 2 years ago

Duplicates

Latest version

Current behavior 😯

Following the example explained here the code works as expected:

<Tabs value={currentTab}>
  <Tab label="Inbox" value="/inbox/:id" to="/inbox/1" component={Link} />
  <Tab label="Drafts" value="/drafts" to="/drafts" component={Link} />
  <Tab label="Trash" value="/trash" to="/trash" component={Link} />
</Tabs>

However, when I try to replicate a similar behavior with the TabsUnstyled this way:

<TabsUnstyled value={currentTab}>
  <TabsListUnstyled>
    <TabUnstyled value="/inbox/:id" to="/inbox/1" component={Link}>
      Inbox
    </TabUnstyled>
    <TabUnstyled value="/drafts" to="/drafts" component={Link}>
      Drafts
    </TabUnstyled>
    <TabUnstyled value="/trash" to="/trash" component={Link}>
      Trash
    </TabUnstyled>
  </TabsListUnstyled>
</TabsUnstyled>

The browser raises the following error:

Warning: React does not recognize theownerStateprop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercaseownerstateinstead. If you accidentally passed it from a parent component, remove it from the DOM element.

I'm passing the Link component from the react-router-dom library to both the Tab and TabUnstyled, if it helps to understand the issue better.

Expected behavior πŸ€”

No errors thrown when using TabUnstyled.

Steps to reproduce πŸ•Ή

Code Sandbox

Context πŸ”¦

No response

Your environment 🌎

  System:
    OS: Linux 5.10 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
  Binaries:
    Node: 18.2.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.9.0 - /usr/local/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: Not Found
  npmPackages:
    @emotion/styled:  10.3.0
    @mui/base: ^5.0.0-alpha.76 => 5.0.0-alpha.79
    @mui/icons-material: ^5.6.1 => 5.6.2
    @mui/material: ^5.6.1 => 5.6.4
    @mui/private-theming:  5.6.2
    @mui/styled-engine-sc: ^5.6.1 => 5.6.1
    @mui/system: ^5.6.4 => 5.6.4
    @mui/types:  7.1.3
    @mui/utils:  5.6.1
    @types/react: ^18.0.3 => 18.0.9
    react: ^18.0.0 => 18.1.0
    react-dom: ^18.0.0 => 18.1.0
    styled-components: ^5.3.5 => 5.3.5
    typescript: ^4.6.4 => 4.6.4
andrejstas commented 2 years ago

I'm getting the same errors when using MenuUnstyled, MenuItemUnstyled and InputUnstyled components, also using the latest @mui/base package (5.0.0-alpha.82)

michaldudak commented 2 years ago

We added the component/components props, so developers can customize the components (slots) used inside the "owner" unstyled component. To be able to customize based on the internal state of the component, the ownerState is passed into any custom component you provide. Obviously, as you noticed, this can create an issue if a component is used that simply spreads all the props it receives. To work around this problem, wrap the Link component in a custom one that will stop spreading the ownerState prop, like so:

const LinkWrapper = React.forwardRef(function LinkWrapper(props, ref) {
  const { ownerState, ...other } = props;
  return <Link {...other} ref={ref} />;
});

We need to update the docs to state what's going on more clearly.

Please let me know if it helps.


@andrejstas could you please show concrete examples of your use cases (a codesandbox would be great)? We are evaluating different solutions to this problem but need to know how developers customize the components.

zzev commented 2 years ago

Yes @michaldudak, it's working now. However, I had to put // @ts-ignore twice to prevent Typescript from complaining, which I think is not ideal.

const LinkWrapper = React.forwardRef((props, ref) => {
  // @ts-ignore
  const { ownerState, ...other } = props;

  // @ts-ignore
  return <Link {...other} ref={ref} />;
});

Here's the updated example.

michaldudak commented 2 years ago

We are in process of adding types for slots of all the unstyled components. Tab hasn't been covered yet, but once it's done, you won't need // @ts-ignore.

ibrahimAboelsuod commented 2 years ago

Doing this: const { ownerState, ...other } = props as unknown as SelectUnstyledProps<any> & { ownerState: any }; makes you avoide using // @ts-ignore

oliviertassinari commented 1 year ago

@michaldudak Since we did 3 PRs on this #35654, #35655, #35668, should this issue be opened? It seems that it's still something worth fixing.


Regarding the solution, why not use the slots.Root vs. component difference to solve it? We could make so that owerState is provided to slots and only. We could also make the component prop be provided with the as prop to slots.Root. I mean, right now, with MUI Base, providing both doesn't work, while shouldn't it work like in Material UI?

We could expect this to work:

import SliderUnstyled from '@mui/base/SliderUnstyled';

function Slider(props) {
  return <SliderUnstyled slots={{ Root, Rail, Track, Thumb }} />
}

<Slider component="span" />

Right now, the component span replaces the Root, which holds the style, so breaks the rendering. I would expect this React tree: Slider > SliderUnstyled > Root > span rather than Slider > SliderUnstyled > span.

michaldudak commented 1 year ago

Since we did 3 PRs on this https://github.com/mui/material-ui/pull/35654, https://github.com/mui/material-ui/pull/35655, https://github.com/mui/material-ui/pull/35668, should this issue be opened? It seems that it's still something worth fixing.

Sure, good point.

Regarding the solution, why not use the slots.Root vs. component difference to solve it? We could make so that owerState is provided to slots and only. We could also make the component prop be provided with the as prop to slots.Root. I mean, right now, with MUI Base, providing both doesn't work, while shouldn't it work like in Material UI?

I'm afraid this would increase the complexity of the overall structure without bringing many gains. We've already seen that the flatter structure we currently have does work well in many use cases. If there is a voice in the community that real-world applications need this additional level of abstraction, we can consider it. As for now, my recommendation is to use hooks whenever building a component library that should be customized further (such as Joy UI) and unstyled components for "end-products".

oliviertassinari commented 1 year ago

increase the complexity of the overall structure

@michaldudak Fair point, I think that the solution we pick should have extra attention to the bundle size and runtime cost.

michaldudak commented 1 year ago

We've recently discussed the topic extensively and come up with this solution: We will introduce a helper function that will wrap 3rd party components and discard ownerState from their props. This way, we won't introduce new props, new contexts wrapping each component or any breaking changes.

The usage will look as follows:

import { Link } from 'react-router-dom';

const LinkSlot = createSlot(Link); 

// ...

  return (
    <TabsUnstyled value={currentTab}>
      <TabsListUnstyled>
        <TabUnstyled value="/inbox/:id" to="/inbox/1" component={LinkSlot}>
          Inbox
        </TabUnstyled>
        <TabUnstyled value="/drafts" to="/drafts" component={LinkSlot}>
          Drafts
        </TabUnstyled>
        <TabUnstyled value="/trash" to="/trash" component={LinkSlot}>
          Trash
        </TabUnstyled>
      </TabsListUnstyled>
    </TabsUnstyled>
  );
jinjilynn commented 1 year ago

I have changed the styled engine to "styled-components", and add the code below to resolve problems like this once for all

    <StyleSheetManager shouldForwardProp={isPropValid}>
      <RouterProvider router={router} />
    </StyleSheetManager>
ever-dev commented 1 year ago

It looks like this is happening when using styled-components v6, it doesn't show up when using v5.3.9

mnajdova commented 1 year ago

We just merged the prepareForSlot util. If you are using third party components in some of the slots and you don't need to use the ownerState you can just wrap the component with the prepareForSlot function. See e.g.: https://deploy-preview-38138--material-ui.netlify.app/base-ui/getting-started/customization/#system-PrepareForSlot.js

import * as React from 'react';
import { prepareForSlot } from '@mui/base/utils';
import { Button } from '@mui/base/Button';
import Link from 'next/link';

const LinkSlot = prepareForSlot(Link);

export default function PrepareForSlot() {
  return (
    <Button href="/" slots={{ root: LinkSlot }}>
      Home
    </Button>
  );
}

It will be available with the next release: @mui/base@5.0.0-beta.11

michaldudak commented 1 year ago

With #38138 merged, I consider the issue closed. Feel free to comment if there's more to be done.

oliviertassinari commented 1 year ago

I explored the problem a bit more coming from https://github.com/mui/material-ui/pull/38568#discussion_r1299373572.

As far as I understand it, using the Unstyled Button only makes sense when you need your design system to support <button> and custom host elements. So once you style the Base UI Button, you still need to expose a way to change the host, otherwise, you would use a button.

This issue raised by the author, also demonstrates how wiring the <Tabs> to the URL is a frequent use case, where you also need to optimize for changing the host element.

I could find two different classes of ways to style the Base UI, taking the button to illustrate: https://codesandbox.io/p/sandbox/hungry-austin-rxxfyl?file=/app/page.tsx:55,28

"use client";

import * as React from "react";
import { prepareForSlot } from "@mui/base/utils";
import { Button as ButtonUnstyled, ButtonProps } from "@mui/base/Button";
import { styled } from "@mui/system";
import Link from "next/link";

const LinkSlot = prepareForSlot(Link);

const Button1Style = styled(ButtonUnstyled)({
  background: "blue",
  color: "#fff",
});

function Button1(props: ButtonProps & { component: React.ElementType }) {
  const { component, ...other } = props;
  return <Button1Style slots={{ root: component }} {...other} />;
}

const Button2Style = styled("button")({
  background: "blue",
  color: "#fff",
});

function Button2(props: ButtonProps & { component: React.ElementType }) {
  const { component, ...other } = props;
  return (
    <ButtonUnstyled
      slots={{ root: Button2Style }}
      slotProps={{ root: { as: component } as any }}
      {...other}
    />
  );
}

export default function PrepareForSlot() {
  return (
    <div>
      <Button1 href="/" component={LinkSlot}>
        Button 1
      </Button1>
      <Button2 href="/" component={LinkSlot}>
        Button 2
      </Button2>
    </div>
  );
}

This don't feel great:

  1. It's quite some boilerplate, Button 2 is even more, quite challenging to figure out
  2. It's a learning curve when coming from Joy UI or Material UI component prop
  3. It doesn't support component prop inheritance, Next.js props fail in TypeScript

So coming back to

I'm afraid this would increase the complexity of the overall structure without bringing many gains.

I lean a bit toward the feeling that the complexity needs to exist, that it's better to have it in the Base UI source than in people's design system codebase.

3 options that could work https://github.com/mui/material-ui/pull/38138#discussion_r1299417564.

kmefeu commented 10 months ago

I'm trying to add some unit tests on my components that use Material UI. I already have tried the solutions above none of them seems to work for me. I have styled components installed on my project because I need some custom components as well.

   "@mui/icons-material": "^5.11.16",
    "@mui/material": "^5.14.5",
    "@mui/styled-engine": "^5.12.0",
    "@mui/styled-engine-sc": "^5.12.0",

My tests are always throwing this error.

console.error
Warning: React does not recognize the `ownerState` prop on a DOM element. If you intentionally want it to appear in the 
DOM as a custom attribute, spell it as lowercase `ownerstate` instead. If you accidentally passed it from a parent 
component, remove it from the DOM element.

This is the component that seems to be throwing the error. It is a custom PassordField. The prepareForSlot function should be able to stop the warning on tests, shouldn't it?

import { prepareForSlot } from "@mui/base/utils";
import Visibility from "@mui/icons-material/Visibility";
import VisibilityOff from "@mui/icons-material/VisibilityOff";
import {
  FormControl,
  IconButton,
  InputAdornment,
  InputLabel,
  OutlinedInput,
} from "@mui/material";
import { useState } from "react";

const PasswordField = (props: any) => {
  const [toggle, setToggle] = useState(false);

  return (
    <FormControl fullWidth sx={{ marginTop: 1.5 }}>
      <InputLabel>Password</InputLabel>
      <OutlinedInput
        {...props}
        type={toggle ? "text" : "password"}
        endAdornment={
          <InputAdornment position="end">
            <IconButton
              aria-label="toggle password visibility"
              onClick={() => setToggle((s) => !s)}
              edge="end"
            >
              {toggle ? <VisibilityOff /> : <Visibility />}
            </IconButton>
          </InputAdornment>
        }
      />
    </FormControl>
  );
};

export default prepareForSlot(PasswordField);

The code is running normally only in the tests I have those annoying warnings. Thanks for your attention

eusouoviana commented 9 months ago

I am getting a similar error:

React does not recognize the `slotProps` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `slotprops` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

When using the DatePicker.