-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[DatePicker] Supports value link and openDialog() method #1213
Conversation
Without actually using it and just looking at the code, this is the behavior I was expecting. (For valueLink) |
Hey @marnusw thank for the pull request! I looked over the code and I like the addition of the openDialog() method, this solves one of the concerns I had. However, in my current project I want to place a button in the AppBar that opens the date-picker-dialog. However, the API is forcing me to place a UITextfield in the AppBar which doesn’t look very good in my opinion. So in my use case the proposed API doesn’t work. The current implementation of the date-picker-dialog component is more flexible; it lets the user decide whether to implement a textfield or not. So I think that exposing the date-picker-dialog will support more use cases without making the API significantly more difficult which I think should be the goal of any API in general. Let me know what you think of my comments and thanks again for helping in improving the date-picker API. |
Thanks for the feedback @gappture. I your case it does make sense to expose the dialog as well. Let's see what @hai-cea thinks. |
*/ | ||
function isValid(value) { | ||
return value instanceof Date; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can go in date-time.js and probably should be renamed to isDateObject
- https://github.com/callemall/material-ui/blob/master/src/utils/date-time.js#L154
Thanks @marnusw - I just had a few comments on your PR. Also, to answer your question about focus... I don't believe the current date picker is keyboard accessible. Now that I think about it, I think the text-field should be able to gain keyboard focus and if the user keys in enter or space, the date picker dialog should open. I think it can be addressed in a future PR. |
@hai-cea, thanks for your feedback. I've addressed it which led to a few other minor changes which I think are also for the better. |
[DatePicker] Supports value link and openDialog() method
Thanks @marnusw ! |
First-off, this PR adds support for
valueLink
as requested in mui/mui-x#7524. The implementation is based on how this is handled inTextField
and supporting controlled inputs previously added in #1170 is now much cleaner, in my opinion, as well.Secondly, perhaps as an alternative to #1206 and #1210, perhaps in addition, two methods are exposed to parent components:
openDialog()
andfocus()
. The function of the former is obvious from the name, the latter is an alias for the same method matching theTextField
api for more generic handling in higher order components. This is quite an effective way to open the dialog when a button is clicked etc. Since the date should (always?) be displayed thereafter, I think it makes sense to tie the dialog closely to a text field, but perhaps there are other use cases that do warrant exposing the dialog as in #1210.(I guess this should technically have been separate PRs, if you like one change but not the other I'll change it accordingly.)
I've looked at also opening the dialog on focus as the comment in the code suggests should be done. This turned out to be rather tricky though: With the focus on the text field being blurred again immediately tabbing breaks down as focus shifts to the
body
, and one can also not detect ifthis
was the tabbed to input in_handleWindowKeyUp()
. Opening the modal in_handleInputFocus
causes it to open briefly and then close again, I couldn't really figure out why. The trouble, I guess, is the dual focus and tap events that are triggered. I got it working by delaying response to the focus event for 150ms and clearing the timer if the tap event also occurs, but this hardly seems ideal. My final thoughts are that it might be best to leave thefocus
on the text field, and the pass down custom styling to it so it doesn't look like it's focused which I'm guessing is the intended outcome. Not entirely sure why though?In addition to @hai-cea, I'd love to hear your comments @JAStanton and @gappture.
(I'll be adding docs for these changes as well before this gets merged if it is decided to do so.)