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

[DatePicker] Add keyboard support to inline mode #5030

Closed
wants to merge 15 commits into from

Conversation

caesay
Copy link

@caesay caesay commented Aug 19, 2016

When container=inline and keyboardEnabled=true on a DatePicker component, this enables keyboard support. You can tab in, type a date, tab out. Also, clicking directly on the control also focuses the textbox.

This was originally opened at PR #4945, but i messed up the branch and had to close the PR and force-push the branch so I can't re-open the old one.

Closes mui/mui-x#6232
Related #5727

@@ -182,6 +209,9 @@ class DatePicker extends Component {
* (get the current system date while doing so)
* else set it to the currently selected date
*/
if (this.shouldHandleKeyboard())

Choose a reason for hiding this comment

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

The comment above this is a little messed up, it actually belongs to line 215.

@oliviertassinari oliviertassinari added the component: date picker This is the name of the generic UI component, not the React module! label Oct 19, 2016
@dasloss
Copy link

dasloss commented Nov 26, 2016

Any expectation timeframe on when this gets merged? Thanks!

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Dec 3, 2016
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. Sorry for the long review delay.
I couldn't try out the demo as this PR is based on an old commit. We might need to rebase it.
However, I'm not sure we gonna move forward with this PR, we want to focus on bug fixes for the master branch and the migration of components on the next branch.

@@ -158,6 +173,12 @@ class DatePicker extends Component {
});
}

componentDidMount() {
const node = ReactDOM.findDOMNode(this.refs.root);
Copy link
Member

Choose a reason for hiding this comment

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

Why accessing the DOM element when we can use the synthetic React events system?

onFocus={this.handleFocus}
onFocus={this.handleInputFocus}
onBlur={this.handleInputBlur}
onKeyDown={this.handleKeyDown}
Copy link
Member

Choose a reason for hiding this comment

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

That's a breaking change, users can no longer override the onKeyDown property.
That's definitely not the only breaking change.

/>
<DatePickerDialog
DateTimeFormat={DateTimeFormat}
autoOk={autoOk}
useLayerForClickAway={!this.shouldHandleKeyboard()}
anchorEl={this.refs.root}
Copy link
Member

Choose a reason for hiding this comment

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

What's the use case behind this change?

event.preventDefault();
return;
}

if (this.props.onTouchTap) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't users still be able to use onTouchTap?

// and also because it doesn't use the browser's timezone.
const parts = filtered.split('-');
if (parts.length === 3)
dt = new Date(parts[0], parts[1] - 1, parts[2]); // Note: months are 0 based
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to allow users using a different formatting pattern.

@dasloss
Copy link

dasloss commented Dec 4, 2016 via email

@oliviertassinari
Copy link
Member

The date picker is almost completely useless
without this fix because it takes 10x longer to enter a date without the
keyboard.

I get that, let's try to move that PR forward 👍 .

@allenwq
Copy link

allenwq commented Jan 17, 2017

@caesay @oliviertassinari Hope this get merged too. Thanks a lot !

@oliviertassinari oliviertassinari added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 21, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 2, 2017

I'm closing this PR as quite old now. The corresponding issue is right here mui/mui-x#6232.
@caesay Thanks for trying to make things go forward 👍 .

@reggiepangilinan
Copy link

Any news when will this be merged? :) we are eager to use it. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: date picker This is the name of the generic UI component, not the React module! PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DatePicker] Use keyboard to write date
7 participants