Migrate to Popper from Popover #1850
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mui-org/material-ui-pickers/1js21bv2h |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
One difference I am seeing – Popover doesn't return the focus back to the open button when closed. Any ideas @oliviertassinari |
There was a problem hiding this comment.
I did a quick review. I'm happy that we move forward in this direction :).
One difference I am seeing – Popover doesn't return the focus back to the open button when closed. Any ideas @oliviertassinari?
I have also noticed that it breaks the scroll position.
lib/src/_shared/PickerPopper.tsx
Outdated
| '&:focus': { | ||
| outline: 'auto', | ||
| [IS_TOUCH_DEVICE_MEDIA]: { | ||
| outline: 0, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
We can remove redundant default for simplicity until we solve the root issue.
| '&:focus': { | |
| outline: 'auto', | |
| [IS_TOUCH_DEVICE_MEDIA]: { | |
| outline: 0, | |
| }, | |
| }, | |
| [IS_TOUCH_DEVICE_MEDIA]: { | |
| outline: 0, | |
| }, |
| transformOrigin: 'bottom center', | ||
| }, | ||
| }), | ||
| { name: 'MuiPickersPopper' } |
There was a problem hiding this comment.
s or not? It doesn't match the name of the file. Which one should be fix?
| { name: 'MuiPickersPopper' } | |
| { name: 'MuiPickerPopper' } |
There was a problem hiding this comment.
I think we will have some kind of name consolidation PR, so it will be easier to find all names by "MuiPickers"
There was a problem hiding this comment.
Sounds great, I love fighting entropy with consistency 😁
There was a problem hiding this comment.
Ok so if we refer to #1778, I think that we can use MuiPickersPopper, but rename the file to
lib/src/_shared/PickersPopper.tsx
| const handlePopperRef = useForkRef(paperRef, innerRef); | ||
|
|
||
| useGlobalKeyDown(open, { | ||
| [keycode.Esc]: onClose, |
There was a problem hiding this comment.
Do we need to listen globally to the Escape key? What prevent to scope the listener to the container?
There was a problem hiding this comment.
It is not always focused on popper (DateRangePicker)
There was a problem hiding this comment.
The date range picker case makes me think of mui/material-ui#17279.
For the date picker, I was under the assumption that the component will always close on blur. Hence, we can listen to the key event on the root.
lib/src/wrappers/DesktopWrapper.tsx
Outdated
|
|
||
| return ( | ||
| <WrapperVariantContext.Provider value="desktop"> | ||
| <KeyboardDateInputComponent {...DateInputProps} containerRef={ref} /> | ||
| <KeyboardDateInputComponent {...DateInputProps} containerRef={inputRef} /> |
There was a problem hiding this comment.
Shouldn't listen to the "Input" rather than the "container"? The popup open bellow the helper text, I believe it should open below the <input /> element.
There was a problem hiding this comment.
Maybe it's a good opportunity to solve #1768? I don't know.
|
@dmtrKovalenko any idea on the next release time line? Just curious as we're checking this PR and waiting for it to hit. Thanks! Great library! |
lib/src/_shared/PickerPopper.tsx
Outdated
| '&:focus': { | ||
| outline: 'auto', | ||
| [IS_TOUCH_DEVICE_MEDIA]: { | ||
| outline: 0, | ||
| }, | ||
| }, |
| transformOrigin: 'bottom center', | ||
| }, | ||
| }), | ||
| { name: 'MuiPickersPopper' } |
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
|
Since which version this change will be available? |
|
It is already available with the latest v4 alpha |
This PR closes #1426 and also closes #1835