Mobile & desktop components #1433

dmtrKovalenko · 2019-12-25T13:39:52Z

A part of vNext #1293

…nent

vercel · 2019-12-25T13:39:57Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click on the icon next to each commit.

cypress · 2019-12-25T13:43:57Z

Test summary

48 • 0 • 0 • 0

Run details


Project	material-ui-pickers
Status	Passed
Commit	`fd6fd39`
Started	Jan 16, 2020 12:34 PM
Ended	Jan 16, 2020 12:35 PM
Duration	00:57 💡
OS	Linux Debian - 9.8
Browser	Electron 61

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

… variant

…lidation in mobile keyboard view

oliviertassinari

The small details I have noticed:

Regarding the codesandbox iframe of the getting started page. It's slow to load. What's the advantage over an embedded demo?
The "Custom spacing" demo, I think that we could remove it. I believe checking for theme.spacing in the source yields +80% of the value. I doubt we will see a regression anytime soon. I imagine having it has helped to solve the underlying issue. But now?
cmd + click on the docs sidenav links doesn't work (open new tab). But it's already handled in http://material-ui.com/, so we should be fine.
disabled seems to be the default prop value (false) in the API docs.
core and pickers use a different naming convention for the descriptions. It would be great to unify them.
There is an alignment issue in the /demo/datetime-picker#basic-usage
Something feels off with the outer spacing: /demo/datepicker#basic-usage
In /demo/timepicker#basic-usage, it would be awesome to include a demo based on this approach: https://material-ui.com/components/autocomplete/#disabled-options.

The important chunks I have noticed:

The desktop version blocks the scroll and moves the focus away from the input.
I think that we should support a defaultValue prop. We have recently introduced a new utility to help you do so: useControlled. Once we do, I don't see any reason for value and onChange to be required.
autoOk: it seems that the majority of the libraries I can benchmark with have it enabled by default, including the default date picker on chrome. Shouldn't we change the default value? And if we do then, we need to rename the prop: to prefix it with disable -> disableAutoOk so we can keep false as the defaut value (a naming convention implemented for all the component, and inspired by the HTML 5 spec for attributes). Now, because the Autocomplete has the same feature, I would propose that we name the prop identically (to increase consistency). Hence my proposal for the following in RFC: material-ui-pickers v4 #1293 (comment), you can find 2 other renaming proposals I would love to get your feedback on (allowKeyboardControl and initialFocusedDate).
```
/**
 * If `true`, the popup won't close when a value is selected.
 *
 * from (autoOk)
 * default false
 */
disableCloseOnSelect: PropTypes.bool,
```
They are a couple of props that don't enforce the boolean naming approach (to always have false as a default). I think that it would be great to change them. We have it documented in https://material-ui.com/guides/api/#property-naming.
I suspect the Autocomplete and the desktop version of the Datepicker have the potential to share a bunch of the same logic. We could even consider making a shared hook, but maybe we should wait for a 3 use case, like a color picker or tree-select.
In any case, the parts I see that should/could have the same answer for: Popper, disableCloseOnSelect, disableOpenOnFocus, keyboard escape, autoComplete, onInputChange, how we expose the TextField. The objective is both to increase consistency and improve the UX.
The keyboard feels slow to move, in dev and with a single month display (I fear what it would be with 2 or 3 months displayed at once). I think that we should try to use a pure DOM-based approach (bypassing React), like http://react-day-picker.js.org/ is doing. https://airbnb.io/react-dates/ has the same issue. It's not great (but I can't find any open issue related to it on react-dates; so I would vote for ignoring the problem, for now, unless it would require an important change of the logic, in which case, as we are diving deep into it, now, might be the best time).
I strongly think that we should remove onMonthChange support for promises. If the user quicky change between months, x requests will be fired, we have no guarantees on the order the network will resolve them (unless I have missed something). We might very well display information for the wrong month.
In practice, removing the support seems to be 90% about updating the demo.
For the spinning issue, I think that a loading prop would be better, enabling the support of more use cases. speaking of the loading state. I think that we could center it (it would solve the scrolling issue).
On the other hand, I think that it would be better to leave the loading implementation with a customization capability. I don't think that it's a frequent use case. Does it justify bundling the circular progress for it?

By the way, you mentioned that you would need 90 hours to make significant progress.
Man, if you want to dedicate more time to it, over another one, it's perfectly fine. This is the type of component that people tend not to write themselves and is very frequently used, so having something very polished can make a huge difference 😍.
I'm wondering if working on different components in parallel, alone, is a better strategy than working on fewer components at the same time, but in small teams of two.

The initiatives I have seen and I have loved

The responsive approach is cool.
The second picker support is cool.
The progress made with the desktop support

oliviertassinari · 2020-01-10T13:51:45Z

docs/package.json

@@ -12,19 +12,21 @@
  "keywords": [],
  "author": "",
  "license": "ISC",
+  "engines": {
+    "node": "12.x"


I'm wondering if we need to mention the engine. For instance, we don't mention it on the main documentation. But we do for the published packages ("node": ">=8.0.0")

It is docs package.json. It is required for zeit now deployments

oliviertassinari · 2020-01-10T13:52:28Z

docs/pages/demo/datepicker/AdvancedKeyboard.example.jsx

+  const [selectedDate, handleDateChange] = useState(new Date());
+
+  return (
+    <Fragment>


Suggested change

<Fragment>

oliviertassinari · 2020-01-10T14:15:37Z

docs/pages/demo/datepicker/AdvancedKeyboard.example.jsx

+        mask="____/__/__"
+        keyboardIcon={<EventNoteIcon />}
+        value={selectedDate}
+        onChange={date => handleDateChange(date)}


For consistency, I think that we should always expose the event as the first argument.

/** * Callback fired when the value changes. * * @param {object} event The event source of the callback * @param {DateIOType} value */ onChange: PropTypes.func,

#1293 (comment)

We cannot use event in onChange because this feature can be fired from

keyboard input (we have event)

clicking ok button (we have event, but it is MouseEvent so I think we already cannot pass 2 different event types)

pressing enter with open datepicker (again different event type)

selecting date in the last available view <-- Here we don't have an event

Again passing event will make more confusion

oliviertassinari · 2020-01-10T14:26:55Z

docs/pages/getting-started/usage.mdx

@@ -13,6 +13,8 @@ import { CODESANDBOX_EXAMPLE_ID } from '_constants';

 #### Quick start

+**Note** `onChange` has a second parameter. So if you will not do `onChange={date => handleDateChange(date)}` you will get the console warning that [useState hook does not accept a second callback argument.](https://github.com/facebook/react/issues/14174)


I don't understand. What's the purpose of the note?

There were 5 issues opened because of this warning. I decided to put this as a note

oliviertassinari · 2020-01-10T14:27:42Z

docs/pages/getting-started/usage.mdx

-      <DatePicker value={selectedDate} onChange={handleDateChange} />
-      <TimePicker value={selectedDate} onChange={handleDateChange} />
-      <DateTimePicker value={selectedDate} onChange={handleDateChange} />
+      <DatePicker value={selectedDate} onChange={date => handleDateChange(date)} />


For reference, we use the following approach in the other components:

Suggested change

<DatePicker value={selectedDate} onChange={date => handleDateChange(date)} />

<DatePicker value={selectedDate} onChange={(event, date) => { handleDateChange(date) }} />

The main incentive for not using implicit returns is to reduce the cost of changes, it avoids having to look at what the logic does (not to break a potentially returned value) before introducing a new line in the handler (I believe that even if using TypeScript, people have to do the effort to check).

I don't think it is will help somebody, but I don't care I can change the examples

oliviertassinari · 2020-01-10T14:37:06Z

lib/.size-snapshot.json

-    "gzipped": 16402,
+    "bundled": 108303,
+    "minified": 61808,
+    "gzipped": 16662,


I don't think that this value represents reality. It seems to ignore the cost of the core component dependencies. I strongly think that it should take the cost of @material-ui/core and date-io into account. Otherwise, we can blindly include a large number of dependencies. I suspect the "real" size is more in the order of 80kB (16 pickers + 20 styles + 35 the other components + 10 date-io) for somebody only using the pickers. Which is about x4 more than we can hope for looking at https://bundlephobia.com/result?p=react-datepicker. I suspect we will be able to find leverages to improve this part :).

What about React? Given that people don't have the choice to use it, I think that, this time, we can remove it from the equation.

What about prop-types, I suspect its cost is also not taken into account because it's marked as a peer dependency. Which, I think that we should also fix. Now, the good news is that @eps1lon has built a more advanced bundle size tracking in https://github.com/mui-org/material-ui (we were using size-limit) before that. They we will be able to leverage once we host the pickers in the mono-repository.

This also raised another point I don't understand. Why is @date-io/core a peer dependency?
I think that they are only two cases that justify a peer dependency: 1. the dependency has a singleton, 2. the dependency is optional. I believe 1. and 2. don't apply.

Note that @material-ui/core should be a peer dependency because of 1. (the styles's theme propagation: context).

Make sure that we have a timepicker - which is entierly different control and requires (I think more then 20% of bundlesize)

@date-io/core has no bundle size. We have optional dependency @date-io/{library} and @date-io/core is used to orchestrate these dependencies and make sure that some of the library is installed. Users don't need to install it -> it is a dependency of each @date-io/{lib} and has no bundle size

oliviertassinari · 2020-01-10T14:58:26Z

lib/src/__tests__/test-utils.tsx

+    <ThemeProvider theme={createMuiTheme()}>
+      <MuiPickersUtilsProvider utils={UtilClassToUse}>{element}</MuiPickersUtilsProvider>
+    </ThemeProvider>
+  );

 export const shallowRender = (render: (props: any) => React.ReactElement<any>) => {


I think that it would be great to migrate all the shallow tests to e2e tests. I haven't looked at how they are used.

dmtrKovalenko added 4 commits December 25, 2019 14:46

Move keyboard managing functionality out from separate hook and compo…

ea57e24

…nent

Remove outdated imports

cd12e74

Deal with typescript generics

d83bfd7

Fix build errros

ca2eead

dmtrKovalenko added the next 😏 label Dec 25, 2019

dmtrKovalenko self-assigned this Dec 25, 2019

dmtrKovalenko added 3 commits December 25, 2019 17:21

Improve type inferences

619115f

Split components by mode

75bdb60

Split to Mobile, Desktop, Static and Responsive components by wrapper…

a9b8183

… variant

vercel bot deployed to staging December 25, 2019 17:33 View deployment

dmtrKovalenko changed the title ~~Feature/mobile & desktop components~~ Mobile & desktop components Dec 25, 2019

Fix docs not working with new components

56b7c2b

vercel bot deployed to Preview January 1, 2020 19:18 View deployment

dmtrKovalenko added 4 commits January 4, 2020 16:16

Adopt date-io v2

5d7b2c5

Fix autosubmit logic for desktop variant

13c1509

Update TimePicker and DateTimepicker documentation examples

e99dfcd

Update date-io version

27b7f6d

vercel bot deployed to Preview January 4, 2020 18:21 View deployment

dmtrKovalenko added 2 commits January 4, 2020 20:25

Remove ts-ignores

df88e41

Fix ts build errors

c555eed

vercel bot deployed to Preview January 4, 2020 18:31 View deployment

dmtrKovalenko added 3 commits January 5, 2020 11:34

Fix console warns

d718dce

Fix responsivenness for DateTimePicker

e72f7c0

Fix some tests

64cb0b7

vercel bot deployed to Preview January 6, 2020 15:47 View deployment

Fix DateTimePicker tests

5381e9a

vercel bot deployed to Preview January 6, 2020 16:49 View deployment

Regenerate lockfile

3ca6d20

dmtrKovalenko added 2 commits January 10, 2020 13:09

Fix prop types generation

99ba757

Move inputValue state down from usePickerState hook

1a1699b

vercel bot deployed to Preview January 10, 2020 11:37 View deployment

dmtrKovalenko added 2 commits January 10, 2020 18:40

Enable working mobile keyboard input from dialog

30abd38

Move mobile keyboard open state up to the pickerState and fix date va…

769ec6c

…lidation in mobile keyboard view

vercel bot deployed to Preview January 10, 2020 17:33 View deployment

dmtrKovalenko requested a review from oliviertassinari January 10, 2020 17:33

oliviertassinari reviewed Jan 10, 2020

View reviewed changes

dmtrKovalenko added 2 commits January 14, 2020 15:48

Move prop-types to deps, add @types/react to dependencies

966d711

Remove useStaticState

d5116a3

vercel bot requested a deployment to Preview January 16, 2020 10:29 Pending

dmtrKovalenko mentioned this pull request Jan 16, 2020

Changed month and year format to correspond to latest date-io #1453

Closed

vercel bot deployed to Preview January 16, 2020 10:32 View deployment

Update cypress docker image

7cc9b0e

vercel bot temporarily deployed to Preview January 16, 2020 10:38 Inactive

Fix incorrect executor reference

2b4f2f1

vercel bot temporarily deployed to Preview January 16, 2020 10:40 Inactive

Updarte version of cireclci cypress orb

7295bfa

vercel bot deployed to Preview January 16, 2020 10:42 View deployment

dmtrKovalenko added 2 commits January 16, 2020 14:13

Fix tests

6ccc1c9

Fix displaying issues

d28a981

vercel bot deployed to Preview January 16, 2020 12:21 View deployment

Fix ts error

fd6fd39

vercel bot deployed to Preview January 16, 2020 12:31 View deployment

dmtrKovalenko marked this pull request as ready for review January 16, 2020 16:45

dmtrKovalenko requested a review from oliviertassinari January 16, 2020 16:45

dmtrKovalenko merged commit 59a5d12 into next Jan 16, 2020

dmtrKovalenko deleted the feature/Mobile-&-Desktop-components branch January 16, 2020 18:17

Misiu mentioned this pull request Jan 17, 2020

RFC: material-ui-pickers v4 #1293

Closed

oliviertassinari mentioned this pull request Feb 21, 2020

Allow setting loading status manually. #1159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile & desktop components #1433

Mobile & desktop components #1433

dmtrKovalenko commented Dec 25, 2019 •

edited

Loading

vercel bot commented Dec 25, 2019

cypress bot commented Dec 25, 2019 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Jan 10, 2020 •

edited

Loading

dmtrKovalenko Jan 10, 2020

oliviertassinari Jan 10, 2020

oliviertassinari Jan 10, 2020

dmtrKovalenko Jan 10, 2020

oliviertassinari Jan 10, 2020

dmtrKovalenko Jan 10, 2020

oliviertassinari Jan 10, 2020

dmtrKovalenko Jan 10, 2020

oliviertassinari Jan 10, 2020 •

edited

Loading

oliviertassinari Jan 10, 2020

dmtrKovalenko Jan 10, 2020

oliviertassinari Jan 10, 2020

		@@ -13,6 +13,8 @@ import { CODESANDBOX_EXAMPLE_ID } from '_constants';

		#### Quick start

		Note `onChange` has a second parameter. So if you will not do `onChange={date => handleDateChange(date)}` you will get the console warning that [useState hook does not accept a second callback argument.](https://github.com/facebook/react/issues/14174)

	<DatePicker value={selectedDate} onChange={date => handleDateChange(date)} />
	<DatePicker value={selectedDate} onChange={(event, date) => { handleDateChange(date) }} />

Mobile & desktop components #1433

Mobile & desktop components #1433

Conversation

dmtrKovalenko commented Dec 25, 2019 • edited Loading

vercel bot commented Dec 25, 2019

cypress bot commented Dec 25, 2019 • edited Loading

Test summary

Run details

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

The small details I have noticed:

The important chunks I have noticed:

The initiatives I have seen and I have loved

oliviertassinari Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko commented Dec 25, 2019 •

edited

Loading

cypress bot commented Dec 25, 2019 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Jan 10, 2020 •

edited

Loading

oliviertassinari Jan 10, 2020 •

edited

Loading