Material-ui-pickers: DateRangePicker onChange not triggering during testing

[mnemanja]

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The feature works in the browser, but not during testing.
As far as I've tested it, the onChange of the DateRangePicker component doesn't get triggered, and the same events of the TextField component gets triggered, but not with the correct value.

Expected Behavior 🤔

I'm expecting for the value that I'm asserting through tests to be persisted.

Steps to Reproduce 🕹

I've tried creating a sandbox, but cannot get it to run properly. I'm saying this since the feedback for tests is missing.
https://codesandbox.io/s/daterangepicker-onchange-not-triggering-022cl

Steps:

1.
2.
3.
4.

Context 🔦

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| @material-ui/core | v5.0.0-alpha.5 |
| @material-ui/pickers | v4.0.0-alpha.12 |
| React | v16.13.1 |
| Browser | |
| TypeScript | |
| etc. | |

[Lukas-Kullmann]

So the problem in our (i.e. @mnemanja and mine) tests was that the date range picker is rendered in mobile mode by default.

The picker library uses media queries to determine if you are using a desktop environment. We were running our tests with jest and jsdom 16. It does not implement window.matchMedia. Defining matchMedia made our tests work:

import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

beforeEach(() => {
  // add window.matchMedia
  // this is necessary for the date picker to be rendered in desktop mode.
  // if this is not provided, the mobile mode is rendered, which might lead to unexpected behavior
  Object.defineProperty(window, 'matchMedia', {
    writable: true,
    value: (query: string): MediaQueryList => ({
      media: query,
      // this is the media query that @material-ui/pickers uses to determine if a device is a desktop device
      matches: query === '(pointer: fine)',
      onchange: () => {},
      addEventListener: () => {},
      removeEventListener: () => {},
      addListener: () => {},
      removeListener: () => {},
      dispatchEvent: () => false,
    }),
  });
});

afterEach(() => {
  delete window.matchMedia;
});

it('should be possible to set a start and end date', async () => {
  render(<MyComponent />);

  const startDateInput = await screen.findByRole('textbox', { name: /start date/i });
  const endDateInput = screen.getByRole('textbox', { name: /end date/i });

  userEvent.clear(startDateInput);
  // this has to be typed one number at a time for the formatting to work
  await userEvent.type(startDateInput, '20200106', { delay: 1 });

  userEvent.clear(endDateInput);
  await userEvent.type(endDateInput, '20200109', { delay: 1 });

  expect(screen.getByRole('textbox', { name: /start date/i })).toHaveValue('2020-01-06');
  expect(screen.getByRole('textbox', { name: /end date/i })).toHaveValue('2020-01-09');
});

It would be nice if this behavior (mobile wrapper being used by default) was documented more clearly. And maybe also having a section in the documentation for those testing pitfalls.

[oliviertassinari]

@Lukas-Kullmann Thanks for sharing the solution. Considering this pool, I think that it should default to desktop, not mobile.

What do you think about this diff?

diff --git a/lib/src/wrappers/ResponsiveWrapper.tsx b/lib/src/wrappers/ResponsiveWrapper.tsx
index 3545a1b4..d0220e8c 100644
--- a/lib/src/wrappers/ResponsiveWrapper.tsx
+++ b/lib/src/wrappers/ResponsiveWrapper.tsx
@@ -18,6 +18,12 @@ export interface ResponsiveWrapperProps
   desktopModeMediaQuery?: string;
 }

+/**
+ * useMediaQuery returns false if the matchMedia API isn't supported.
+ * However, we want to optimize for desktop first, not mobile.
+ */
+const supportMatchMedia = typeof window !== 'undefined' && typeof window.matchMedia !== 'undefined';
+
 export const makeResponsiveWrapper = (
   DesktopWrapperComponent: React.FC<DesktopWrapperProps | DesktopTooltipWrapperProps>,
   MobileWrapperComponent: React.FC<MobileWrapperProps>
@@ -36,7 +42,8 @@ export const makeResponsiveWrapper = (
     TransitionComponent,
     ...other
   }) => {
-    const isDesktop = useMediaQuery(desktopModeMediaQuery);
+    // eslint-disable-next-line react-hooks/rules-of-hooks
+    const isDesktop = supportMatchMedia ? useMediaQuery(desktopModeMediaQuery) : true;

     return isDesktop ? (
       <DesktopWrapperComponent

I think that it would also be great to mention the behavior of useMediaQuery in https://material-ui.com/components/use-media-query/#testing.

diff --git a/docs/src/pages/components/use-media-query/use-media-query.md b/docs/src/pages/components/use-media-query/use-media-query.md
index 5587217de..50d23fa16 100644
--- a/docs/src/pages/components/use-media-query/use-media-query.md
+++ b/docs/src/pages/components/use-media-query/use-media-query.md
@@ -63,6 +63,7 @@ You can use [json2mq](https://github.com/akiran/json2mq) to generate media query
 ## Testing

 You need an implementation of [matchMedia](https://developer.mozilla.org/en-US/docs/Web/API/Window/matchMedia) in your test environment.
+If not available, the hook will always return `false`.

 For instance, [jsdom doesn't support it yet](https://github.com/jsdom/jsdom/blob/master/test/web-platform-tests/to-upstream/html/browsers/the-window-object/window-properties-dont-upstream.html). You should polyfill it.
 Using [css-mediaquery](https://github.com/ericf/css-mediaquery) to emulate it is recommended.

[Lukas-Kullmann]

To be honest, I don't like this approach. It still defaults to mobile since it tests for an isDesktop media query. I think defaulting to desktop would mean inverting all of the checks: Using mobileMediaQuery input into the ResponsiveWrapper component, making the default media query @media (pointer: coarse) there and also changing it in all other places.

But in the end, I don't mind that the default variant is mobile. It would just be nice if it was documented better on the website.
I think a good place would be to create testing page in the guides section. It could document the mobile behavior and also that you have to type one character at a time to the input fields in your tests.

Also, it could be clearer documented that the DatePicker, DateRangePicker etc. are responsive by default.
The way I use the website is that I go to the component demos and search for the use case I have. And then I see how it is implemented in the examples. This is great!
The problem is that on the component demo of the date range picker (and also date picker, time picker, etc.) it starts with "basic usage" and then it goes to "responsiveness". If you never used this library, it seems like responsiveness is something you need to configure, not the default.
So if you do not need the responsiveness, you would not look at the source code and see that the responsive DatePicker component is actually the regular DatePicker component. Maybe it would be good to get rid of the "basic usage" and just make the "responsiveness" the basic usage. And give a hint that jsdom does not support media queries by default. So the default on jsdom is mobile.
Or (and this might be controversial) you get rid of the DatePicker component all together and just provide MobileDatePicker, DesktopDatePicker and ResponsiveDatePicker. Then the developers using this library are forced to think about the implications of what they are using. This could of course be implemented in a way that there still is a DatePicker that just wraps ResponsiveDatePicker, but prints a warning in the console in dev mode or something like this. And then it can be removed in version 5.