Inner state for picker modal

[dmtrKovalenko]

This PR closes #1049

Description

This PR make pickers to use the inner state for displaying inside modal. To reduce side-effects for other API. It will still dispatch onChange for keyboard mode but will dispatch onChange for standard pickers only when the date is accepted.

[dmtrKovalenko]

Also closes #1088

[codecov]

Codecov Report

Merging #1095 into next will decrease coverage by 0.13%.
The diff coverage is 94.59%.

@@            Coverage Diff             @@
##             next    #1095      +/-   ##
==========================================
- Coverage   95.14%   95.01%   -0.14%     
==========================================
  Files          52       53       +1     
  Lines        1340     1344       +4     
  Branches      188      178      -10     
==========================================
+ Hits         1275     1277       +2     
- Misses         47       49       +2     
  Partials       18       18

Impacted Files	Coverage Δ
lib/src/Picker/WrappedKeyboardPicker.tsx	100% <ø> (ø)	⬆️
lib/src/_shared/hooks/useKeyDown.ts	72.22% <ø> (ø)	⬆️
lib/src/_shared/hooks/useKeyboardPickerState.ts	76.92% <ø> (ø)	⬆️
lib/src/Picker/WrappedPurePicker.tsx	100% <ø> (ø)	⬆️
lib/src/_shared/hooks/useOpenState.ts	100% <100%> (ø)
lib/src/_shared/WithUtils.tsx	100% <100%> (ø)	⬆️
lib/src/_shared/hooks/usePickerState.ts	92.15% <91.3%> (-1.18%)	⬇️
lib/src/TimePicker/components/Clock.tsx	96% <0%> (-4%)	⬇️
lib/src/TimePicker/TimePickerToolbar.tsx	95% <0%> (-2.5%)	⬇️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81c813e...5ebb355. Read the comment docs.

[TrySound]

Here ok button is missing but month click does not close popup. Is it intentional?

[dmtrKovalenko]

I guess it is unrelated

[Philipp91]

AFAIU, the props object always changes. It's possible, in theory, that the component that passes the props has them memoized, but in all normal circumstances (using JSX, or call sites like this), the props object will be a new object. And useCallback only memoizes when the deps are the same according to Object.is(), so the useCallback() here is ineffective. It can either be removed (if we don't care about memoization) or be fixed:

const {onAccept, onChange} = props;
const acceptDate = useCallback(
  (acceptedDate: MaterialUiPickersDate) => {
    setIsOpen(false);
    onChange(acceptedDate);
     if (onAccept) {
      onAccept(acceptedDate);
    }
  },
  [setIsOpen, onAccept, onChange]
);

If you agree (I might be wrong, I'm very new to hooks), I can send a PR for this and similar occurrences.

[dmtrKovalenko]

Yep, that will be more efficient

[dmtrKovalenko]

It will not be changed if only internal state changed. It anyway it will be cool to have more clear deps for top level hook

[Philipp91]

Off-topic question: Is there a way I can test changes like this one in my own app before they have been released to NPM? I tried cloning the repo, running the build and then pointing the NPM dependency from my app to the lib/build directory. Everything has the right structure, but at runtime it complains that React was loaded twice (which breaks hooks, "Invariant Violation: Invalid hook call.").

[dmtrKovalenko]

You can make a build and link your app manually for the folder lib/build.
Go to lib/build
Run npm link
Go to your app
Run npm link @material-ui/pickers

[Philipp91]

Thanks! I didn't know that npm link exists. The commands you gave lead to the same symlink I had before (just via the global node_modules directory), so it didn't resolve the duplicate-React issue. But knowing that npm link is the canonical way to do this allowed me to research this issue. I tried many things that try to fix the symlink situation, but none of them worked. Instead, I did this:
https://medium.com/@vcarl/problems-with-npm-link-and-an-alternative-4dbdd3e66811

material-ui-pickers/lib/build/ $ npm pack
my-app/ $ npm install ../material-ui-pickers/lib/build/material-ui-pickers-3.1.0.tgz

Obviously that's more annoying for iterating quickly, but it worked for testing this PR. I can confirm that the fix in this PR works for me and can replace the workaround classes discussed in #1049. Thanks a lot!

[pankaz]

Kindly merge #1036 into the "next" branch. The "clear" functionality is breaking in the current "next" branch.