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.

Sign up for GitHub

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

Jump to bottom

[pickers] Do not support unparsed date formats anymore #6170

Merged
merged 19 commits into from
Sep 23, 2022
Merged

[pickers] Do not support unparsed date formats anymore #6170

merged 19 commits into from
Sep 23, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 15, 2022 •
edited
Loading

Closes #5759

@mui-bot
Copy link

mui-bot commented Sep 15, 2022 •
edited
Loading

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 548.7 932.9 814.7 744.14 148.378
Sort 100k rows ms 625.6 1,006.6 746 869.9 155.38
Select 100k rows ms 179.1 272.4 206.9 219.82 32.87
Deselect 100k rows ms 140.6 283.5 197.4 218.08 54.098

Generated by 🚫 dangerJS against 5b0aae2

@flaviendelangle flaviendelangle self-assigned this Sep 16, 2022
@flaviendelangle flaviendelangle added breaking change typescript component: pickers This is the name of the generic UI component, not the React module! and removed typescript labels Sep 16, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 16, 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 Sep 16, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 19, 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 Sep 19, 2022
@flaviendelangle flaviendelangle changed the title [pickers] Do not support unparsed date formats anymore (WIP) [pickers] Do not support unparsed date formats anymore Sep 20, 2022
@flaviendelangle flaviendelangle marked this pull request as ready for review September 20, 2022 06:00
alexfauquette
alexfauquette reviewed Sep 20, 2022
View reviewed changes
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

I read some files, but it would take an eternity to open each file modification to check if modification are just type renaming or logic modification. Are there some specific lines on which you have a doubt?

docs/data/date-pickers/date-picker/date-picker.md
@@ -73,7 +73,7 @@ For ease of use, the date picker will automatically change the layout between po

## Sub-components

Some lower-level sub-components (`CalendarPicker`, `MonthPicker`, and `YearPicker`) are also exported. These are rendered without a wrapper or outer logic (masked input, date values parsing and validation, etc.).
Some lower-level sub-components (`CalendarPicker`, `MonthPicker`, and `YearPicker`) are also exported.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the link with this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

date values parsing

There is no parsing any more, that's the part related to this PR

validation

Not related to this PR but this is false, they do have validation

masked input

Indeed, but I don't think it's worth specifying the the MonthPicker does not have a masked input, it's obvious by looking at the example below

@LukasTy LukasTy added the v6.x label Sep 20, 2022
@LukasTy
Copy link
Member

LukasTy commented Sep 20, 2022

I read some files, but it would take an eternity to open each file modification to check if modification are just type renaming or logic modification.

[...document.querySelectorAll('button.load-diff-button')].forEach(button => button.click());

🙈 😆

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 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 Sep 22, 2022
@flaviendelangle
Copy link
Member Author

I read some files, but it would take an eternity to open each file modification to check if modification are just type renaming or logic modification. Are there some specific lines on which you have a doubt?

The internals mostly, the main point of risk for me is in usePickerState and the shared.ts (for each picker family)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 22, 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 Sep 22, 2022
alexfauquette
alexfauquette approved these changes Sep 22, 2022
View reviewed changes
packages/x-date-pickers-pro/src/MultiInputDateRangeField/useMultiInputDateRangeField.ts

newDateRange[index] = newDate;
const currentDateRange =
valueProp ?? firstDefaultValue.current ?? dateRangePickerValueManager.emptyValue;
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need to clean this value?

Suggested change
valueProp ?? firstDefaultValue.current ?? dateRangePickerValueManager.emptyValue;
dateRangePickerValueManager.cleanValue(valueProp) ?? firstDefaultValue.current ?? dateRangePickerValueManager.emptyValue;

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 think the field need to remove the invalid date.
To be honest I think it's not great on the view either but I did not feel confident removing this security.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 23, 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 Sep 23, 2022
@flaviendelangle flaviendelangle merged commit bffb32c into mui:next Sep 23, 2022
@flaviendelangle flaviendelangle deleted the remove-t-input branch September 23, 2022 15:50
@flaviendelangle flaviendelangle mentioned this pull request Nov 17, 2022
Merged
@flaviendelangle flaviendelangle mentioned this pull request Oct 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexfauquette alexfauquette alexfauquette approved these changes

@LukasTy LukasTy LukasTy approved these changes

Assignees

@flaviendelangle flaviendelangle

Labels
breaking change component: pickers This is the name of the generic UI component, not the React module! v6.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC] Should we keep the different between TInputDate and TDate in v6
4 participants
@flaviendelangle @mui-bot @LukasTy @alexfauquette