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

[DataGrid] Fix DatePicker bug by limiting years to 4 digits #3222

Merged
merged 5 commits into from
Dec 7, 2021

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Nov 19, 2021

Fix #3221

HTML input allows to set more than 4 digits in a year but it seems to be a problem when setting the value so I propose to fix the maximal year to 9999. Probably need to change it around year 8000 ;)

It would probably be possible to avoid the bug without limiting years to 4 digits because filters can accept such dates. But I do not see any value for that and would take more time to find where does the bug comes from.

demo: https://deploy-preview-3222--material-ui-x.netlify.app/components/data-grid/demo/

@sebastianfrey
Copy link
Contributor

sebastianfrey commented Nov 19, 2021

I have noticed, that it is also possible in the input component for filtering date fields to set more then 4 digits in a year.

@alexfauquette
Copy link
Member Author

I've noticed it too, but it does not make the input component buggy. Do you feel it is a bug to be able to set years with 5 digits?

@sebastianfrey
Copy link
Contributor

sebastianfrey commented Nov 20, 2021

@alexfauquette Indeed, it does not make the input component buggy. But I think it does not feel natural and intuitive, when users can enter 5+ digits for the year.

From my perspective it feels like a bug, because in my use case I have to adopt multiple server backends using the grid component. Each of those server backends handles dates differently. That means, some of them may not fail when querying a date field with a date with more than 4 year digits, and some of them will fail. And so I would have to handle the date validation for each of those backends outside of the grid filter component, which is not ideal.

@flaviendelangle
Copy link
Member

We have edge cases where dates with 5 digits can make sense (if I want the starting year of my favorite SF cycles for instance 😄 )
But I suppose people can always create custom date-custom col types with a renderer calling GridEditDateCell with additional props.

@m4theushw
Copy link
Member

For consistency I'm in favor of also not allowing 5-digit years in the filter value. With #3227, it will be easier to add the max prop.

@sebastianfrey
Copy link
Contributor

@flaviendelangle In my opinion the special case is 5 or more digits per year. So if someone has such a requirement, than he/she should create a custom date component. But it's up to you guys, what direction this is going.

@flaviendelangle
Copy link
Member

For consistency I'm in favor of also not allowing 5-digit years in the filter value. With #3227, it will be easier to add the max prop.

Should it be done on this PR ?

@alexfauquette
Copy link
Member Author

@flaviendelangle Il will merge #3227 and do the max digits in this one

@alexfauquette
Copy link
Member Author

@flaviendelangle My bad, I did it in the other PR. So filter limitation is already on master. But I did it wrong. I did not notice the distinction between InputProps and inputProps. That's corrected in last commit

https://github.com/mui-org/material-ui-x/blob/1a4324cbc24723b768ef0544a72b09b272094c16/packages/grid/_modules_/grid/components/panel/filterPanel/GridFilterInputDate.tsx#L50

max: type === 'datetime-local' ? '9999-12-31T23:59' : '9999-12-31',
inputProps: {
max: type === 'datetime-local' ? '9999-12-31T23:59' : '9999-12-31',
...inputProps,
Copy link
Member

@m4theushw m4theushw Nov 29, 2021

Choose a reason for hiding this comment

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

The filter input is based on the TextField so users expect that the props will be forwarded to the root component.

Here you're spreading the top-level inputProps inside InputProps. It's like:

<GridFilterInputDate inputProps={{ min: '...' }} />
-> <TextField InputProps={{ inputProps: { min: '...' } }} /> (this PR)
-> <TextField inputProps={{ min: '...' }} /> (how it should behave)

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 totally agree, but Eslint do not agree with us and complains about duplicate props.
I made some research to find other solutions, but it seems core team recommends it mui/material-ui#10064 (comment)

...(applying ? { endAdornment: <GridLoadIcon /> } : {}),
inputProps: {
max: type === 'datetime-local' ? '9999-12-31T23:59' : '9999-12-31',
...inputProps,
Copy link
Member

Choose a reason for hiding this comment

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

The top-level inputProps is still being applied inside InputProps. The following diff should fix this

diff --git a/packages/grid/_modules_/grid/components/panel/filterPanel/GridFilterInputDate.tsx b/packages/grid/_modules_/grid/components/panel/filterPanel/GridFilterInputDate.tsx
index 7e061a7ec..e8b4f1006 100644
--- a/packages/grid/_modules_/grid/components/panel/filterPanel/GridFilterInputDate.tsx
+++ b/packages/grid/_modules_/grid/components/panel/filterPanel/GridFilterInputDate.tsx
@@ -11,7 +11,7 @@ export type GridFilterInputDateProps = GridFilterInputValueProps &
 export const SUBMIT_FILTER_DATE_STROKE_TIME = 500;
 
 function GridFilterInputDate(props: GridFilterInputDateProps) {
-  const { item, applyValue, type, apiRef, focusElementRef, inputProps, InputProps, ...other } =
+  const { item, applyValue, type, apiRef, focusElementRef, InputProps, ...other } =
     props;
 
   const filterTimeout = React.useRef<any>();
@@ -61,11 +61,11 @@ function GridFilterInputDate(props: GridFilterInputDateProps) {
       inputRef={focusElementRef}
       InputProps={{
         ...(applying ? { endAdornment: <GridLoadIcon /> } : {}),
+        ...InputProps,
         inputProps: {
           max: type === 'datetime-local' ? '9999-12-31T23:59' : '9999-12-31',
-          ...inputProps,
+          ...InputProps?.inputProps,
         },
-        ...InputProps,
       }}
       {...other}
     />

@alexfauquette alexfauquette merged commit 8ac4544 into mui:master Dec 7, 2021
@alexfauquette alexfauquette deleted the limit-year branch December 7, 2021 08:21
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid]DatePickers bugs after saving a year with 5 digits
5 participants