Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pickers] New unstable field components #5504

Merged
merged 63 commits into from
Aug 24, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jul 15, 2022

These components are very early stage, please don't use them in production

Doc Preview of DateField
Doc Preview of DateRangeField

Part of #5531

Wanted interactions

  • Ability to add a day / a month / a year with the "ArrowUp" and "ArrowDown" keys
  • Ability to switch from day to month to year with the "ArrowLeft" and "ArrowRight" keys

Missing @date-io methods

@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Jul 15, 2022
@flaviendelangle flaviendelangle self-assigned this Jul 15, 2022
@mui-bot
Copy link

mui-bot commented Jul 15, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 447.3 795.7 650.5 607.92 137.418
Sort 100k rows ms 570.6 1,096.1 570.6 909.1 191.507
Select 100k rows ms 213.9 299.9 278 262.12 32.464
Deselect 100k rows ms 139.1 306.3 202.1 228 66.256

Generated by 🚫 dangerJS against 842cf5e

@joserodolfofreitas
Copy link
Member

The interactions with arrow keys feel great! Could it somehow block the implementation for a chain of digits as input?

@joserodolfofreitas
Copy link
Member

Btw, where does this POC leave us in terms of a single input implementation for a date range? Would a single input for a range use the same notion used here to isolate the specific 'pieces' of a date?

@flaviendelangle
Copy link
Member Author

Btw, where does this POC leave us in terms of a single input implementation for a date range?

I did not look into it yet
It will definitely make the analysis of the input harder but maybe that's doable.
Let's try to see if it's viable for a the simple use case and then stress test it on harder one.

We also have the date time where we may want to handle it like Retool and open a select, in which case the input must only cover the date part.

@flaviendelangle
Copy link
Member Author

Could it somehow block the implementation for a chain of digits as input?

I did not understand sorry

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Jul 15, 2022

Could it somehow block the implementation for a chain of digits as input?

I did not understand sorry

I raised the question because I noticed you cannot input digits on the current text field POC. It currently only supports the arrow keys, so the question is: Will there be any problem with supporting numeric values as well?

And by "chain of digits", I meant if the user could type numbers like: 1 5 0 7 2 0 2 2 to have an output like 15/07/2022

edit: Just saw the latest update to support numeric values, so the question is now partially answered: we can already input numbers!

@flaviendelangle
Copy link
Member Author

Will there be any problem with supporting numeric values as well?

Yes, I started to work on it.
It raised a certain amount of questions so I'm trying to get a first working draft to discuss trade-offs.
For instance on Telerik, when you have a full letter month, you can still type it's number and it sets the month (type 5 => sets "May")

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Jul 18, 2022

@joserodolfofreitas Concerning the editing, Telerik does not goes from what section of the date to another automatically.

If you are on the day and type "1", "2", "3", it will set the day to "1", then "12", then "3".
To go to the section section you have to use the keyboard arrows.

Spectrum does move automatically to the next section.
For numerical value it is doable. But I'm curious to see how they handle that for full letter months.


This behavior could be customizable if we have users asking for both

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 21, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 21, 2022
@flaviendelangle
Copy link
Member Author

@gerdadesign

Also noting a pattern I've seen of the placeholder labels giving an indication for how many digits the input format is looking for: MM/DD/YYYY or DD/MM/YY

For now the placeholder of an empty section is the name of the section (month for a month section for instance)
Which is really basic and do not support any kind of localization.
We should probably improve this, I'm open to suggestions for the default behavior and the customization options.

@flaviendelangle
Copy link
Member Author

Clearable example does not work correctly. It does not reset the value in the component.

@LukasTy I had a dumb bug where I did not use the controlled value in the component, several of your bugs were probably cause by this error, it is fixed now

@LukasTy
Copy link
Member

LukasTy commented Aug 22, 2022

Clearable example does not work correctly. It does not reset the value in the component.

I had a dumb bug where I did not use the controlled value in the component, several of your bugs were probably cause by this error, it is fixed now

I can confirm that all my mentioned issues have been resolved. 🙌 👍

@LukasTy
Copy link
Member

LukasTy commented Aug 22, 2022

@flaviendelangle What do you think about adding the following diff?
It adds management of Home, End, PageUp and PageDown keys mentioned here as well as some function name alignment.
Note: We currently don't have a clear way to know actual max or min year, hence I did not that Home/End management for it.

diff --git a/packages/x-date-pickers/src/internals/hooks/useField/useField.ts b/packages/x-date-pickers/src/internals/hooks/useField/useField.ts
index 5470f19c..84c01798 100644
--- a/packages/x-date-pickers/src/internals/hooks/useField/useField.ts
+++ b/packages/x-date-pickers/src/internals/hooks/useField/useField.ts
@@ -11,12 +11,13 @@ import {
   UseFieldState,
 } from './useField.interfaces';
 import {
+  AvailableAdjustKeyCode,
   cleanTrailingZeroInNumericSectionValue,
   getMonthsMatchingQuery,
   getSectionValueNumericBoundaries,
   getSectionVisibleValue,
-  incrementDateSectionValue,
-  incrementOrDecrementInvalidDateSection,
+  adjustDateSectionValue,
+  adjustInvalidDateSectionValue,
   setSectionValue,
 } from './useField.utils';
 
@@ -153,7 +154,7 @@ export const useField = <
       }
 
       // Reset the value of the selected section
-      case event.key === 'Backspace': {
+      case event.key === 'Backspace' || event.key === 'Delete': {
         event.preventDefault();
 
         if (readOnly) {
@@ -181,7 +182,12 @@ export const useField = <
       }
 
       // Increment / decrement the selected section value
-      case event.key === 'ArrowUp' || event.key === 'ArrowDown': {
+      case event.key === 'ArrowUp' ||
+        event.key === 'ArrowDown' ||
+        event.key === 'Home' ||
+        event.key === 'End' ||
+        event.key === 'PageUp' ||
+        event.key === 'PageDown': {
         event.preventDefault();
 
         if (readOnly || state.selectedSectionIndexes == null) {
@@ -196,21 +202,21 @@ export const useField = <
 
         // The date is not valid, we have to increment the section value rather than the date
         if (!utils.isValid(activeDate.value)) {
-          const newSectionValue = incrementOrDecrementInvalidDateSection(
+          const newSectionValue = adjustInvalidDateSectionValue(
             utils,
             activeSection,
-            event.key === 'ArrowUp' ? 'increment' : 'decrement',
+            event.key as AvailableAdjustKeyCode,
           );
 
           updateSections(
             setSectionValue(state.sections, state.selectedSectionIndexes.start, newSectionValue),
           );
         } else {
-          const newDate = incrementDateSectionValue(
+          const newDate = adjustDateSectionValue(
             utils,
             activeDate.value,
             activeSection.dateSectionName,
-            event.key === 'ArrowDown' ? -1 : 1,
+            event.key as AvailableAdjustKeyCode,
           );
 
           const newValue = activeDate.update(newDate);
diff --git a/packages/x-date-pickers/src/internals/hooks/useField/useField.utils.ts b/packages/x-date-pickers/src/internals/hooks/useField/useField.utils.ts
index 0dcd3f56..64977a0d 100644
--- a/packages/x-date-pickers/src/internals/hooks/useField/useField.utils.ts
+++ b/packages/x-date-pickers/src/internals/hooks/useField/useField.utils.ts
@@ -37,33 +37,89 @@ export const getDateSectionNameFromFormat = (format: string): DateSectionName =>
   throw new Error(`getDatePartNameFromFormat don't understand the format ${format}`);
 };
 
-export const incrementDateSectionValue = <TDate>(
+export type AvailableAdjustKeyCode =
+  | 'ArrowUp'
+  | 'ArrowDown'
+  | 'PageUp'
+  | 'PageDown'
+  | 'Home'
+  | 'End';
+
+const getDeltaFromKeyCode = (keyCode: Omit<AvailableAdjustKeyCode, 'Home' | 'End'>) => {
+  switch (keyCode) {
+    case 'ArrowUp':
+      return 1;
+    case 'ArrowDown':
+      return -1;
+    case 'PageUp':
+      return 5;
+    case 'PageDown':
+      return -5;
+    default:
+      return 0;
+  }
+};
+
+export const adjustDateSectionValue = <TDate>(
   utils: MuiPickersAdapter<TDate>,
   date: TDate,
   datePartName: DateSectionName,
-  datePartValue: 1 | -1,
+  keyCode: AvailableAdjustKeyCode,
 ) => {
+  const delta = getDeltaFromKeyCode(keyCode);
+  const isStart = keyCode === 'Home';
+  const isEnd = keyCode === 'End';
   switch (datePartName) {
     case 'day': {
-      return utils.addDays(date, datePartValue);
+      if (isStart) {
+        return utils.startOfMonth(date);
+      }
+      if (isEnd) {
+        return utils.endOfMonth(date);
+      }
+      return utils.addDays(date, delta);
     }
     case 'month': {
-      return utils.addMonths(date, datePartValue);
+      if (isStart) {
+        return utils.startOfYear(date);
+      }
+      if (isEnd) {
+        return utils.endOfYear(date);
+      }
+      return utils.addMonths(date, delta);
     }
     case 'year': {
-      return utils.addYears(date, datePartValue);
+      return utils.addYears(date, delta);
     }
     case 'am-pm': {
-      return utils.addHours(date, datePartValue * 12);
+      return utils.addHours(date, delta ? 1 : 1 * 12);
     }
     case 'hour': {
-      return utils.addHours(date, datePartValue);
+      if (isStart) {
+        return utils.startOfDay(date);
+      }
+      if (isEnd) {
+        return utils.endOfDay(date);
+      }
+      return utils.addHours(date, delta);
     }
     case 'minute': {
-      return utils.addMinutes(date, datePartValue);
+      if (isStart) {
+        return utils.setMinutes(date, 0);
+      }
+      if (isEnd) {
+        return utils.setMinutes(date, 59);
+      }
+      return utils.addMinutes(date, delta);
     }
     case 'second': {
-      return utils.addSeconds(date, datePartValue);
+      if (isStart) {
+        return utils.setSeconds(date, 0);
+      }
+      if (isEnd) {
+        return utils.setSeconds(date, 59);
+      }
+      return utils.addSeconds(date, delta);
     }
     default: {
       return date;
@@ -277,13 +333,16 @@ export const getSectionValueNumericBoundaries = <TDate>(
   }
 };
 
-export const incrementOrDecrementInvalidDateSection = <TDate, TSection extends FieldSection>(
+export const adjustInvalidDateSectionValue = <TDate, TSection extends FieldSection>(
   utils: MuiPickersAdapter<TDate>,
   section: TSection,
-  type: 'increment' | 'decrement',
+  keyCode: AvailableAdjustKeyCode,
 ) => {
   const today = utils.date()!;
-  const delta = type === 'increment' ? 1 : -1;
+  const delta = getDeltaFromKeyCode(keyCode);
+  const isStart = keyCode === 'Home';
+  const isEnd = keyCode === 'End';
+  const shouldSetAbsolute = section.value === '' || isStart || isEnd;
 
   switch (section.dateSectionName) {
     case 'year': {
@@ -299,8 +358,8 @@ export const incrementOrDecrementInvalidDateSection = <TDate, TSection extends F
 
     case 'month': {
       let newDate: TDate;
-      if (section.value === '') {
-        if (type === 'increment') {
+      if (shouldSetAbsolute) {
+        if (delta > 0 || isEnd) {
           newDate = utils.endOfYear(today);
         } else {
           newDate = utils.startOfYear(today);
@@ -314,8 +373,8 @@ export const incrementOrDecrementInvalidDateSection = <TDate, TSection extends F
 
     case 'day': {
       let newDate: TDate;
-      if (section.value === '') {
-        if (type === 'increment') {
+      if (shouldSetAbsolute) {
+        if (delta > 0 || isEnd) {
           newDate = utils.endOfMonth(today);
         } else {
           newDate = utils.startOfMonth(today);
@@ -332,7 +391,7 @@ export const incrementOrDecrementInvalidDateSection = <TDate, TSection extends F
       const pm = utils.formatByString(utils.endOfDay(today), section.formatValue);
 
       if (section.value === '') {
-        if (type === 'increment') {
+        if (delta > 0 || isEnd) {
           return pm;
         }
         return am;
@@ -347,8 +406,8 @@ export const incrementOrDecrementInvalidDateSection = <TDate, TSection extends F
 
     case 'hour': {
       let newDate: TDate;
-      if (section.value === '') {
-        if (type === 'increment') {
+      if (shouldSetAbsolute) {
+        if (delta > 0 || isEnd) {
           newDate = utils.endOfDay(today);
         } else {
           newDate = utils.startOfDay(today);
@@ -362,9 +421,9 @@ export const incrementOrDecrementInvalidDateSection = <TDate, TSection extends F
 
     case 'minute': {
       let newDate: TDate;
-      if (section.value === '') {
+      if (shouldSetAbsolute) {
         // TODO: Add startOfHour and endOfHours to adapters to avoid hard-coding those values
-        const newNumericValue = type === 'increment' ? 59 : 0;
+        const newNumericValue = delta > 0 || isEnd ? 59 : 0;
         newDate = utils.setMinutes(today, newNumericValue);
       } else {
         newDate = utils.addMinutes(utils.setMinutes(today, Number(section.value)), delta);
@@ -375,9 +434,9 @@ export const incrementOrDecrementInvalidDateSection = <TDate, TSection extends F
 
     case 'second': {
       let newDate: TDate;
-      if (section.value === '') {
+      if (shouldSetAbsolute) {
         // TODO: Add startOfMinute and endOfMinute to adapters to avoid hard-coding those values
-        const newNumericValue = type === 'increment' ? 59 : 0;
+        const newNumericValue = delta > 0 || isEnd ? 59 : 0;
         newDate = utils.setSeconds(today, newNumericValue);
       } else {
         newDate = utils.addSeconds(utils.setSeconds(today, Number(section.value)), delta);

@flaviendelangle
Copy link
Member Author

I applied @LukasTy suggestion to handle other keys edition

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 23, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 24, 2022
@flaviendelangle flaviendelangle merged commit 5416854 into mui:master Aug 24, 2022
@flaviendelangle flaviendelangle deleted the date-field branch August 24, 2022 12:58
@gerdadesign

This comment was marked as duplicate.

@flaviendelangle
Copy link
Member Author

Not trying to reopen the merged PR, but just continuing this convo! LMK if I should move it elsewhere.

Maybe move it to the issue #5531, it seems to be a good place to discuss cross PR topics around the fields.

@oliviertassinari oliviertassinari added the new feature New feature or request label Aug 24, 2022
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great! I have left a few comments as code comments so we can benefit from threads (#5531) doesn't provide this.


return (
<LocalizationProvider dateAdapter={AdapterDateFns}>
<Stack spacing={2} sx={(theme) => ({ width: theme.spacing(48) })}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make the demo easier to scan:

Suggested change
<Stack spacing={2} sx={(theme) => ({ width: theme.spacing(48) })}>
<Stack spacing={2} sx={{ width: 384 })}>

export default function BasicDateField() {
return (
<LocalizationProvider dateAdapter={AdapterDateFns}>
<DateField label="Basic date field" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

End-users need to click twice to be able to select the year. Compare:

Screen.Recording.2022-08-24.at.22.33.21.mov

https://deploy-preview-5504--material-ui-x.netlify.app/x/react-date-pickers/date-field/#basic-usage

With (better):

Screen.Recording.2022-08-24.at.22.35.41.mov

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a known issue
You have the same in Telerik: https://www.telerik.com/kendo-react-ui/components/dateinputs/datepicker/

The problem is that I select all sections on focus.
And when you click, it fires onFocus before onClick.
So when onClick is fired, the focus the selection caused by the focus is already applied, so it clicks on a fully selected input.

And on a fully selected input, I can't find the cursor position of the click, the cursor just moves to the beginning of the input.

One solution would be to delay the selection caused by the focus and to skip it if a click selection has been done in the meanwhile.

Copy link
Member

@oliviertassinari oliviertassinari Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I select all sections on focus

I would dive into this. I think it's where the problem starts:

const handleInputFocus = useEventCallback(() => {
// TODO: Avoid applying focus when focus is caused by a click
updateSelectedSections(0, state.sections.length - 1);
});

Maybe instead of a "select all" on focus, the browser does it automatically when focusing an input with a Tab, we could look at inputRef.current.selectionEnd - inputRef.current.selectionStart. If the value equals 0 it would mean that it wasn't a keyboard focus, it was a click focus with a caret that has no selections. Then, we can look at where the current selection is on, and select the corresponding segment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a PR to improve this behavior #5901

Comment on lines +15 to +25
<DateField
label="French locale (default format)"
value={value}
onChange={(newValue) => setValue(newValue)}
/>
<DateField
label="French locale (full letter month)"
value={value}
onChange={(newValue) => setValue(newValue)}
format="dd MMMM yyyy"
/>
Copy link
Member

@oliviertassinari oliviertassinari Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with #5531 (comment). We might want to change the placeholder. Say the input is empty:

Screenshot 2022-08-24 at 22 42 44

Can you guess that it should look like this in the end?

Screenshot 2022-08-24 at 22 42 39

https://deploy-preview-5504--material-ui-x.netlify.app/x/react-date-pickers/date-field/#localization

It might be clearer like this for the first textbox:

Screenshot 2022-08-24 at 22 42 02

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date


The second advantage is that it would minimize layout shifts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The placeholder is clearly temporary.

Telerik uses the section type

image

Spectrum uses something closer from the date tokens

image

export default function BasicDateField() {
return (
<LocalizationProvider dateAdapter={AdapterDateFns}>
<DateField label="Basic date field" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow 00?

Screenshot 2022-08-24 at 22 47 14

Maybe we should reset the field when this happens

Screenshot 2022-08-24 at 22 47 18

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know for this one
In Spectrum, if I press 0 it does not write anything, even if the end format will be 01

Press 0 => still render the placeholder (totally ignores the event I think)
Press 1 => renders 01

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow 00?

To be more precise, I was thinking of what would happen when leaving the selection. A native browser seems to turn 00 to 01 when unselecting.

Comment on lines +1 to +22
import * as React from 'react';
import TextField from '@mui/material/TextField';

import { DateFieldProps } from './DateField.interfaces';
import { useDateField } from './useDateField';

type DateFieldComponent = (<TInputDate, TDate = TInputDate>(
props: DateFieldProps<TInputDate, TDate> & React.RefAttributes<HTMLInputElement>,
) => JSX.Element) & { propTypes?: any };

export const DateField = React.forwardRef(function DateField<TInputDate, TDate = TInputDate>(
inProps: DateFieldProps<TInputDate, TDate>,
ref: React.Ref<HTMLInputElement>,
) {
const { inputRef, inputProps } = useDateField<
TInputDate,
TDate,
DateFieldProps<TInputDate, TDate>
>(inProps);

return <TextField ref={ref} inputRef={inputRef} {...inputProps} />;
}) as DateFieldComponent;
Copy link
Member

@oliviertassinari oliviertassinari Nov 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the simplicity of the code, I almost feel like it would be better to expose this userland. Not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is exposed and should be documented soon

Do you think we should not expose the component ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK great to know.

Do you think we should not expose the component ?

I can't make my mind on this. I think that it's not impossible that we could be better off without this abstraction, with the rationale that lower level API makes easier customizations.

As a side note, I wonder if developers won't expect to be able to target MuiDateField in the theme.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/mui/mui-x/blob/next/packages/x-date-pickers/src/DateField/DateField.tsx

The component has a little more logic know and supports theme overrides as you mentionned.
And we have multi-input range fields with a lot more logic (those I would clearly not remove them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants