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

[Popper] Material/Joy exposing sx prop? #29604

Closed
2 tasks done
jpmtrabbold opened this issue Nov 10, 2021 · 9 comments · Fixed by #31833
Closed
2 tasks done

[Popper] Material/Joy exposing sx prop? #29604

jpmtrabbold opened this issue Nov 10, 2021 · 9 comments · Fixed by #31833
Labels
component: Popper The React component. See <Popup> for the latest version. new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@jpmtrabbold
Copy link

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

When I try to set the style of a Popper using sx, the prop is not there (at least Typescript is not showing anything).

Expected behavior 🤔

To have an sx prop on the Popper component, so I can set the style of the popper div - and to make it consistent with the other components.

Steps to reproduce 🕹

Steps:

  1. try to use the sx prop on a Popper component
  2. typescript will throw a compilation error

Context 🔦

There's a popper coming out of a button that is inside an AppBar. I need to set the zIndex of that popper to at least the same zIndex as the AppBar so it appears on top of it.

Your environment 🌎

`npx @mui/envinfo`
System:
    OS: macOS 12.0.1
  Binaries:
    Node: 14.18.0 - ~/.nvm/versions/node/v14.18.0/bin/node
    Yarn: 1.22.15 - ~/.nvm/versions/node/v14.18.0/bin/yarn
    npm: 6.14.15 - ~/.nvm/versions/node/v14.18.0/bin/npm
  Browsers:
    Chrome: 95.0.4638.69 (used this one)
    Edge: Not Found
    Firefox: 91.0.2
    Safari: 15.1
  npmPackages:
    @emotion/react: ^11.5.0 => 11.5.0 
    @emotion/styled: ^11.3.0 => 11.3.0 
    @mui/core:  5.0.0-alpha.54 
    @mui/icons-material: ^5.1.0 => 5.1.0 
    @mui/lab: ^5.0.0-alpha.54 => 5.0.0-alpha.54 
    @mui/material: ^5.1.0 => 5.1.0 
    @mui/private-theming:  5.1.0 
    @mui/styled-engine:  5.1.0 
    @mui/styles: ^5.1.0 => 5.1.0 
    @mui/system:  5.1.0 
    @mui/types:  7.1.0 
    @mui/utils:  5.1.0 
    @types/react: ^17.0.34 => 17.0.34 
    react: ^17.0.2 => 17.0.2 
    react-dom: ^17.0.2 => 17.0.2 
    typescript: ^4.4.4 => 4.4.4 
@jpmtrabbold jpmtrabbold added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 10, 2021
@siriwatknp

This comment has been minimized.

@siriwatknp siriwatknp added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 10, 2021
@oliviertassinari oliviertassinari added component: Popper The React component. See <Popup> for the latest version. discussion and removed status: waiting for author Issue with insufficient information labels Nov 10, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 10, 2021

@jpmtrabbold The Popper component of Material is defined as:

https://github.com/mui-org/material-ui/blob/b0996ed3d0de77b17906570925ffb1d48b83534c/packages/mui-material/src/Popper/index.js#L1

So it doesn't come with any style or sx. Still, the Popper component does have a host HTML element:

https://github.com/mui-org/material-ui/blob/b0996ed3d0de77b17906570925ffb1d48b83534c/packages/mui-base/src/Popper/Popper.js#L166

that developers might want to customize with the sx prop, and not a styled() wrapper.

So I think that the question is about should Joy/Material wrap the Popper component with a styled() to bring the sx prop, or not? It's related to #23220.

@oliviertassinari oliviertassinari changed the title Popper is not exposing sx prop [Popper] Material/Joy exposing sx prop? Nov 10, 2021
@oliviertassinari oliviertassinari added the new feature New feature or request label Nov 10, 2021
@jpmtrabbold
Copy link
Author

@oliviertassinari yep - as a workaround, I wrapped it with styled() to use the sx prop.

I believe that it should be wrapped with styled by default - to ensure consistency among all mui components IMHO.

@siriwatknp

This comment has been minimized.

@michaldudak
Copy link
Member

I believe that it should be wrapped with styled by default - to ensure consistency among all mui components IMHO.

I agree with this - I'm in favor of following the principle of least surprise. If other components allow styling with sx, so should the Popper.

What's about @mui/base expose a factory function like createPopper() and let the design system passed a tooltip component?

I'd rather follow the pattern we've already been using - expose the components prop in PopperUnstyled and customize the Popper this way.

@oliviertassinari
Copy link
Member

For context, the approach proposed by @siriwatknp is what React Toolbox tried in 2016 to solve what we try to solve with unstyled. I personally find the components prop more flexible.

@siriwatknp
Copy link
Member

@michaldudak @oliviertassinari The way to move forward is to create a material Popper version with default styled-component (so that the consumer can pass sx prop to it). I mark this as "good to take".

@siriwatknp siriwatknp added ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed discussion labels Nov 12, 2021
@pranit1
Copy link

pranit1 commented Feb 9, 2022

Hi I am new to Open Source, can I work on this issue?

@michaldudak
Copy link
Member

Sure, go ahead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popper The React component. See <Popup> for the latest version. new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants