mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.12k stars 1.28k forks source link

[DatePicker] Custom popper component is displayed below dialog modal #10035

Open DoubleDebug opened 1 year ago

DoubleDebug commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/3f6y2h

Steps:

  1. Click on "Enter date" button (second one)
  2. Click on date icon to select date
  3. Popper component is displayed "below" the dialog modal

Current behavior 😯

When you add a DatePicker component inside of a Dialog, and you also add a custom Popper component to the slots prop, the popper appears one layer below the dialog, which makes it non-interactable.

Expected behavior 🤔

The Popper appears on top of the dialog and is interactable, as it's a child component of the dialog.

Context 🔦

This is easily fixable by setting the z-index CSS property of the popper. Here's an example:

<DatePicker
    slots={{
        popper: (props) => <Popper {...props} sx={{ ...props.sx, zIndex: 1400 }} />
    }}
/>

However, this should be the default case and should be implemented in the library itself.

Your environment 🌎

npx @mui/envinfo ``` System: OS: Windows 10 10.0.19045 Binaries: Node: 16.16.0 - C:\Program Files\nodejs\node.EXE Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD npm: 9.7.2 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: Not Found Edge: Spartan (44.19041.1266.0), Chromium (115.0.1901.200) npmPackages: @emotion/react: ^11.11.1 => 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: ^5.0.0-beta.10 => 5.0.0-beta.10 @mui/core-downloads-tracker: 5.14.4 @mui/lab: ^5.0.0-alpha.139 => 5.0.0-alpha.139 @mui/material: ^5.14.4 => 5.14.4 @mui/private-theming: 5.14.4 @mui/styled-engine: 5.13.2 @mui/system: ^5.14.4 => 5.14.4 @mui/types: 7.2.4 @mui/utils: 5.14.4 @mui/x-data-grid: 6.11.1 @mui/x-data-grid-pro: ^6.11.1 => 6.11.1 @mui/x-date-pickers: ^6.11.1 => 6.11.1 @mui/x-license-pro: 6.10.2 @types/react: ^18.2.20 => 18.2.20 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.1.6 => 5.1.6 ```

Order ID or Support key 💳 (optional)

No response

flaviendelangle commented 1 year ago

Hi,

This does not fix your bug, but you should never pass render functions:

slots={{
  // Invalid
  popper: props => <Popper {...props} />,

  // Valid
  popper: Popper,
}}

Otherwise your component will remount on every render. Concerning your problem, why you need to override the popper ?

Our default popper defines its zIndex:

const PickersPopperRoot = styled(MuiPopper, {
  name: 'MuiPickersPopper',
  slot: 'Root',
  overridesResolver: (_, styles) => styles.root,
})(({ theme }) => ({
  zIndex: theme.zIndex.modal,
}));

But if you override the component, you have to re-define it yourseff. If you just want to pass some props to the popper without loosing the default behaviors, you should probably use slotProps instead.

LukasTy commented 1 year ago

IMHO, I do agree with the issue author, I would not expect that I will need to override the zIndex of a library component in order to be able to use it in my app. Let's take a generic example not concerning X or pickers. I try to use a popper inside of a dialog and it fails to show unless I provide an explicit zIndex. 🙈 https://codesandbox.io/s/popper-in-dialog-problem-m4nrm7?file=/Demo.tsx This to me looks more like a MUI Core (Material) problem rather than pickers one. I think that the most logical solution in this case is to add the same default zIndex to the popper component. 🤔

DoubleDebug commented 1 year ago

@flaviendelangle Thanks for the tip regarding render functions! And yes, for my concrete use case, I can use slotProps, leave the default Popper component and that would work. But as soon as I override the Popper component, the zIndex gets messed up.

@LukasTy Yes, I completely agree. This issue isn't necessarily related to DatePickers. The thing is - that's where I encountered it so I thought it's related. 😃 Do you want me to move this issue to MUI Core?

LukasTy commented 1 year ago

@DoubleDebug No worries, we'll try to manage it ourselves. 👌 I'll just try confirming with the core team on their perspective. @mui/core do you agree that this is an actual issue with the existing Popper implementation that needs mending?

oliviertassinari commented 1 year ago

Do you agree that this is an actual issue with the existing Popper implementation that needs mending?

@LukasTy I think:

  1. We have to decide on whether we push forward with Popper only available in Base UI or continue with a fork of it for Material UI, in which case, the Material UI's Popper version could indeed have some default elevation; Autocomplete also has zIndex: theme.zIndex.modal. It would be a breaking change though, likely.

  2. There is an ongoing thread with @DiegoAndai https://github.com/mui/material-ui/pull/38021 about either slot default value should be re-exported. I think this issue is another case where it makes more sense to fix it with, than point 1:

import { Box, Dialog, Popper } from "@mui/material";
import { DatePicker, PickersPopper, LocalizationProvider } from "@mui/x-date-pickers";
import { AdapterDayjs } from "@mui/x-date-pickers/AdapterDayjs";

const MyPopper = (props) => <PickersPopper {...props} />;

export const CustomPopper = ({ show, onClose }) => {
  return (
    <Dialog open={show} onClose={onClose}>
      <Box display="flex" p={4}>
        <LocalizationProvider dateAdapter={AdapterDayjs}>
          <DatePicker
            slots={{
              popper: MyPopper
            }}
          />
        </LocalizationProvider>
      </Box>
    </Dialog>
  );
};
  1. It's better to involve the component maintainers or its owner. I think we have reached a point where @mui/core for specific problems doesn't work. https://www.notion.so/mui-org/Components-Utils-721b15652aeb4b7d95e8cb496b835eab lists @mnajdova and @mj12albert. Now, @michaldudak worked on the Popper, so maybe we should actually document each component maintainer individually (need more granularity).