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] Fix onDismiss handler in MobileDatePicker #30768

Merged
merged 11 commits into from
Feb 3, 2022
Merged

[Pickers] Fix onDismiss handler in MobileDatePicker #30768

merged 11 commits into from
Feb 3, 2022

Conversation

Ashish2097
Copy link
Contributor

@Ashish2097 Ashish2097 commented Jan 25, 2022 •
edited by hbjORbj

Closes #30731

@mui-bot
Copy link

mui-bot commented Jan 25, 2022 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against 381d1c1

@danilo-leal danilo-leal added the component: pickers This is the name of the generic UI component, not the React module! label Jan 25, 2022
@danilo-leal danilo-leal changed the title Fix/30731 mobile picker cancel [Pickers] Fix mobile picker cancel button Jan 25, 2022
mnajdova
mnajdova reviewed Jan 26, 2022
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Let's add test for it

@mnajdova mnajdova added the PR: needs test The pull request can't be merged label Jan 26, 2022
@Ashish2097
Copy link
Contributor Author

Hi @mnajdova, added the test cases.

@hbjORbj hbjORbj added bug 🐛 Something doesn't work and removed PR: needs test The pull request can't be merged labels Jan 28, 2022
@hbjORbj hbjORbj changed the title [Pickers] Fix mobile picker cancel button [Pickers] Fix cancel behaviour in MobileDatePicker Jan 28, 2022
@hbjORbj hbjORbj changed the title [Pickers] Fix cancel behaviour in MobileDatePicker [Pickers] Fix onDismiss handler in MobileDatePicker Feb 2, 2022
@hbjORbj hbjORbj force-pushed the fix/30731-mobile-picker-cancel branch from 989f4d4 to 381d1c1 Compare February 2, 2022 11:44
@hbjORbj
Copy link
Member

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
siriwatknp reviewed Feb 3, 2022
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);

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the explanation 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we need one more additional state :)

siriwatknp
siriwatknp approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 This is awesome for the first contribution!

@hbjORbj hbjORbj merged commit 6eb0003 into mui:master Feb 3, 2022
@mnajdova
Copy link
Member

mnajdova commented Feb 3, 2022

@flaviendelangle FYI

@flaviendelangle
Copy link
Member

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

@flaviendelangle flaviendelangle mentioned this pull request Feb 3, 2022
Merged
2 tasks
@oukunan oukunan mentioned this pull request Feb 15, 2022
Closed
1 task
@yyq1025 yyq1025 mentioned this pull request Mar 23, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phantomcosmonaut phantomcosmonaut phantomcosmonaut left review comments

@hbjORbj hbjORbj hbjORbj left review comments

@siriwatknp siriwatknp siriwatknp approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

@flaviendelangle flaviendelangle Awaiting requested review from flaviendelangle

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MobileDatePicker's Cancel button does not work.
8 participants
@Ashish2097 @mui-bot @hbjORbj @mnajdova @flaviendelangle @phantomcosmonaut @siriwatknp @danilo-leal