[Pickers] Fix `onDismiss` handler in `MobileDatePicker` #30768

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

Merged

hbjORbj merged 11 commits into mui:master from Ashish2097:fix/30731-mobile-picker-cancel

Feb 3, 2022

Contributor

Ashish2097 commented Jan 25, 2022 •

edited by hbjORbj

Loading

Closes #30731

mui-bot commented Jan 25, 2022 •

edited

Loading

Details of bundle changes

Generated by 🚫 dangerJS against 381d1c1

danilo-leal added the scope: pickers label

danilo-leal changed the title ~~Fix/30731 mobile picker cancel~~ [Pickers] Fix mobile picker cancel button

mnajdova reviewed

View reviewed changes

Member

mnajdova left a comment

Let's add test for it

mnajdova added the PR: needs test label

Contributor Author

Ashish2097 commented Jan 27, 2022

Hi @mnajdova, added the test cases.

Ashish2097 requested a review from mnajdova

January 27, 2022 10:28

hbjORbj reviewed

View reviewed changes

packages/mui-lab/src/MobileDatePicker/MobileDatePicker.test.tsx Outdated Show resolved Hide resolved

hbjORbj reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

hbjORbj reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

hbjORbj reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

hbjORbj reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/wrappers/WrapperProps.ts Outdated Show resolved Hide resolved

hbjORbj reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

hbjORbj added type: bug and removed PR: needs test labels

hbjORbj changed the title ~~[Pickers] Fix mobile picker cancel button~~ [Pickers] Fix cancel behaviour in MobileDatePicker

Ashish2097 commented

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

Ashish2097 commented

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

phantomcosmonaut reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/PickersModalDialog.tsx Show resolved Hide resolved

phantomcosmonaut reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

phantomcosmonaut reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts Outdated Show resolved Hide resolved

hbjORbj requested a review from siriwatknp

February 2, 2022 10:42

hbjORbj changed the title ~~[Pickers] Fix cancel behaviour in MobileDatePicker~~ [Pickers] Fix onDismiss handler in MobileDatePicker

hbjORbj requested a review from flaviendelangle

February 2, 2022 11:04

Ashish Duklan and others added 6 commits

February 2, 2022 11:23


          fix(mui-picker): mui#30731- revert changes on cancel

a72f04e


          fix: prettier white space

5ca7edd


          test(picker): mobile picker cancel button test

4b2b8a7


          fix: prettier

de92e64


          Apply suggestions from code review

Co-authored-by: Benny Joo <[email protected]>


          fix: changed type at declaration

d50e85d

Ashish Duklan and others added 5 commits

February 2, 2022 11:23


          fix: type fix

a21327a


          Improve and keep consistency

affbc82


          Remove unnecessary code

361d54d


          Remove code

1fd5b70


          Refactor code

381d1c1

hbjORbj force-pushed the fix/30731-mobile-picker-cancel branch from 989f4d4 to 381d1c1 Compare

February 2, 2022 11:44

Contributor

hbjORbj commented Feb 2, 2022

I can confirm in this codesandbox that the issue is fixed.

Codesandbox before the fix: https://codesandbox.io/s/eloquent-feather-iznb0?file=/src/Demo.tsx

siriwatknp reviewed

View reviewed changes

packages/mui-lab/src/internal/pickers/hooks/usePickerState.ts

Comment on lines +73 to +74

    
                const [initialDate, setInitialDate] = React.useState<TDateValue>(draftState.committed);

Member

siriwatknp Feb 3, 2022

This means initialDate will always be the first value of draftState.committed. Is this the expected behavior? (I don't know the whole implementation but creating another state kinda concern me a bit because it can create another bug where the state is not updated)

Contributor

hbjORbj Feb 3, 2022

It is set to the value of draftState.committed when the component first loads, and afterwards, it is newly set every time user clicks "OK". (We call setInitialDate inside acceptDate function, only if needClosePicker is true. You can check my last commit).

Member

siriwatknp Feb 3, 2022

I see, thanks for the explanation 👍

Member

mnajdova Feb 3, 2022

Yeah, we need one more additional state :)

siriwatknp approved these changes

View reviewed changes

Member

siriwatknp left a comment

👍 This is awesome for the first contribution!

hbjORbj merged commit 6eb0003 into mui:master

Member

mnajdova commented Feb 3, 2022

@flaviendelangle FYI

Member

flaviendelangle commented Feb 3, 2022

Thanks, I'm reproducing it on MUI-X 👍

flaviendelangle mentioned this pull request

[DatePicker] Initial commit mui/mui-x#3451

Merged

2 tasks

oukunan mentioned this pull request

[DateTimePicker] Fix onChange(null) of DesktopDateTimePicker on click away #31093

Closed

1 task

yyq1025 mentioned this pull request

[DatePicker] Fix input value issues #31929

Closed

1 task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

siriwatknp siriwatknp approved these changes

mnajdova Awaiting requested review from mnajdova

flaviendelangle Awaiting requested review from flaviendelangle

+2 more reviewers

phantomcosmonaut phantomcosmonaut left review comments

hbjORbj hbjORbj left review comments

Labels

scope: pickers type: bug