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

[base] Add prepareForSlot util #38138

Merged
merged 11 commits into from
Aug 10, 2023
Merged

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jul 24, 2023

Related to #32882

The PR adds a prepareForSlot util that can be used with third-party libraries' components if they are use inside the slots prop. Currently if a third-party component is used directly in a slot, there is a React warning shown: "React does not recognize the ownerState prop on a DOM element." In order for this to be fixed, so far developers needed to create wrapper component so that they would intercept the ownerState prop in order to not be propagated to the underlying component. This new util is basically doing this behind the scene.

 import NextLink from 'next/link';
 import { Button } from '@mui/base/Button';
+import { prepareForSlot } from '@mui/base/utils';

+const Link = prepareForSlot(NextLink);

 export default function App() {
   return (
-  <Button slots={{ root: NextLink }} href="/">Home</Button> // 💣 ownerState is propagated to the DOM <a/> element
+  <Button slots={{ root: Link }} href="/">Home</Button>
   );
 }

@mnajdova mnajdova added the package: base-ui Specific to @mui/base label Jul 24, 2023
@mui-bot
Copy link

mui-bot commented Jul 24, 2023

@mnajdova mnajdova marked this pull request as ready for review July 25, 2023 10:09
@@ -0,0 +1,21 @@
import * as React from 'react';

export default function createSlot<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hard part - naming. I'm not sure about "createSlot", as the function does not create a slot.
Something like "prepareForSlot" would be closer to the truth.

Also, please add JSDocs.

Copy link
Member

@siriwatknp siriwatknp Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have to compare createSlot and prepareForSlot, I would go with createSlot. It's more intuitive and easy to remember (as a user).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it kind of goes against what we try to teach - slots are places where we insert components, so you can't really create them on your own.
@samuelsycamore, what do you think? Maybe something that's not a real word, like "slotify", work?

Copy link
Contributor

@samuelsycamore samuelsycamore Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof this is a hard one. Do we need to keep the basic structure of actionObject? Here are some spitball ideas to consider:

  • customizeSlot
  • insertSlot
  • insertCustomSlot
  • wrapCustomSlot

But all of these kind of muddy the definition of a slot, as you point out. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • createSlotComponent - you are not creating the slot, but the component to be used inside
  • prepareForSlot - you prepare the component to be used as a slot, maybe shortly prepareSlot
    Two more options. I think if it is up to me I would go with createSlotComponent, it's a bit longer, but descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut says to go with createSlotComponent. I think it offers the most accurate description of what you're doing: creating the component that will go in the slot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget what I said before! 😅 I think prepareForSlot is the most accurate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized it's the first name Michal proposed too :D Ok let's go with it.

packages/mui-base/src/utils/createSlot.test.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/utils/createSlot.spec.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/utils/createSlot.spec.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/utils/createSlot.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2023
@mnajdova mnajdova requested a review from samuelsycamore August 8, 2023 10:37
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got no remarks on the implementation. From the names you proposed I like prepareForSlot better, but I'll let @samuelsycamore decide.

mnajdova and others added 3 commits August 9, 2023 13:25
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 9, 2023
@mnajdova mnajdova changed the title [base] Add createSlot util [base] Add prepareForSlot util Aug 9, 2023
@mnajdova
Copy link
Member Author

mnajdova commented Aug 9, 2023

Updated to prepareForSlot

Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs look good! I think we landed on the best name in the end. 😁

Comment on lines 141 to 156

You can use this object to style your component.

:::warning
When inserting a component from a third-party library into a slot, you may encounter this warning: `"React does not recognize the ownerState prop on a DOM element."`
This is because the custom component isn't prepared to receive the `ownerState` like a built-in library component would be.
:::

If you need to use the `ownerState` to propagate some props to a third-party component, you must create a custom wrapper for this purpose.
But if you don't need the `ownerState` and just want to resolve the error, you can use the `prepareForSlot` utility:

{{"demo": "PrepareForSlot.js", "defaultCodeOpen": true}}

### Customizing slot props

Use the `slotProps` prop to customize the inner component props.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-topic. This page feels a bit out of context in the getting started area. This feels more like a how-to guide or an integration guide.

Comment on lines +1 to +14
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>
);
}
Copy link
Member

@oliviertassinari oliviertassinari Aug 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this example doesn't fully work in real life, for example, this returns a TypeScript error:

<Button href="/" slots={{ root: LinkSlot }} prefetch={false}>

https://nextjs.org/docs/pages/api-reference/components/link#prefetch

Maybe we should remove the prepareForSlot() API and instead document this?

import * as React from 'react';
import { Button, ButtonProps } from '@mui/base/Button';
import Link, { LinkProps } from 'next/link';

const ButtonLink = React.forwardRef<HTMLAnchorElement, ButtonProps & LinkProps>(
  function ButtonLink(props, ref) {
    const {
      // @ts-expect-error
      ownerState,
      ...other
    } = props;
    return <Button ref={ref as any} slots={{ root: Link }} {...other} />;
  },
);

export default function PrepareForSlot() {
  return (
    <ButtonLink href="/" prefetch={false}>
      Home
    </ButtonLink>
  );
}

Or maybe this goes back to the need for a component prop, it's working with Material UI:

Screenshot 2023-08-20 at 20 25 49

For the sake of considering more options, Reshape' button is a span unless a onClick is provided, allowing to compose a > span. it feels quite strange though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dropped the implicit polymorphism, but it's possible to make it explicit, as described in https://mui.com/base-ui/react-button/#custom-structure

The example should use it:

<Button<typeof LinkSlot> href="/" slots={{ root: LinkSlot }} prefetch={false}>

Copy link
Member

@oliviertassinari oliviertassinari Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know this. This sounds quite interesting.
What happens once a component is customized with styled(), does it still work?

Could we apply this to our example? As I think the initial point I raise stays true.

Actually, once applied repeatedly, it feels like this asks for a wrapper component to keep it more nominal. Once you introduce a wrapper, you are better not to use prepareForSlot as it creates fewer React components, better performance.

Copy link
Member

@michaldudak michaldudak Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens once a component is customized with styled(), does it still work

Nope, this is a limitation of TypeScript. Similarly, in Material UI, you can't use component prop on a component returned from styled. It's one of the cases where render props or asChild work better.

Once you introduce a wrapper, you are better not to use prepareForSlot

That's right. prepareForSlot is meant to be used with bare 3rd party components. Once you wrap it and can prevent forwarding the ownerState, you don't have to use the function.

Could we apply this to our example?

Sure: #38640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants