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

auto focus on calendar input when opening picker #438

Merged

Conversation

onlyann
Copy link
Contributor

@onlyann onlyann commented Nov 14, 2018

The current behaviour of the picker is that in order to enter a date manually, one has to click on the picker input twice.

This PR changes the behaviour of the picker to focus the DateInput input element instead.

@ant-design-bot
Copy link

ant-design-bot commented Nov 14, 2018

Deploy preview for rc-calendar failed.

Built with commit 622ff9b

https://app.netlify.com/sites/rc-calendar/deploys/5c2c36ffdaf1cb0008d6a437

@onlyann
Copy link
Contributor Author

onlyann commented Nov 14, 2018

This should fix ant-design/ant-design#11848

@mischa-s
Copy link

That lovely moment when you are confused about trying to use a module and the pull request exists to fix exactly what you are working on 😍 ... now just gotta wait for the merge 👼

@LinusCenterstrom
Copy link

Any status on this? I've had some users requesting this behavior. Would be happy to help if there is anything that needs doing to get this merged.

@onlyann
Copy link
Contributor Author

onlyann commented Dec 2, 2018

Any status on this? I've had some users requesting this behavior. Would be happy to help if there is anything that needs doing to get this merged.

Hey @zombieJ ! Would you kindly review the PR when you find the time. Thanks in advance!

@afc163 afc163 requested a review from zombieJ December 3, 2018 16:25
@zombieJ
Copy link
Member

zombieJ commented Dec 14, 2018

Hi, thanks for PR and sorry for delay.
Currently picker input support direction key operation to change the date in panel. Apply the auto focus, we should also support user to handle the direction key. (Maybe, tap tab to focus on the panel to enable it):
kapture 2018-12-14 at 14 41 43

@onlyann
Copy link
Contributor Author

onlyann commented Dec 15, 2018

Hi, thanks for PR and sorry for delay.
Currently picker input support direction key operation to change the date in panel. Apply the auto focus, we should also support user to handle the direction key. (Maybe, tap tab to focus on the panel to enable it):
kapture 2018-12-14 at 14 41 43

Thank you for the feedback @zombieJ .
Yes, that is one way to do it.
One drawback is that it then forces the user to tab twice to move out of the date picker.

Instead of tabbing to the panel, direction keys could be handled even if the date input is in focus, effectively removing the input target short-circuit in the calendar onKeyDown.
It would remove the ability to move the text cursor on the input.

Thoughts?

EDIT: I pushed a new commit with the described change

@onlyann onlyann force-pushed the pr/auto-focus-calendar-input branch from 01e24fd to a2c2100 Compare December 15, 2018 11:46
@codecov-io
Copy link

codecov-io commented Dec 15, 2018

Codecov Report

Merging #438 into master will increase coverage by 0.53%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #438      +/-   ##
=========================================
+ Coverage   88.77%   89.3%   +0.53%     
=========================================
  Files          10      10              
  Lines         659     664       +5     
  Branches      189     190       +1     
=========================================
+ Hits          585     593       +8     
+ Misses         63      61       -2     
+ Partials       11      10       -1
Impacted Files Coverage Δ
src/mixin/CommonMixin.js 100% <100%> (+12%) ⬆️
src/date/DateInput.js 83.33% <100%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f4522...622ff9b. Read the comment docs.

@zombieJ
Copy link
Member

zombieJ commented Dec 17, 2018

@onlyann, thanks for adjust. Currently I test left & right operation in input. Panel works but cursor in input not work:
kapture 2018-12-17 at 12 10 29

@onlyann
Copy link
Contributor Author

onlyann commented Dec 18, 2018

@zombieJ . Yes, this is what I meant by

It would remove the ability to move the text cursor on the input.

I am not sure it would make too much sense to have the left/right arrows to move both the input cursor and the panel.

@zombieJ
Copy link
Member

zombieJ commented Dec 18, 2018

hi @onlyann ,
It needn't move both of them. Just input auto focus and tab to focus on panel operation. User still can shift + tab back to input.

@onlyann onlyann force-pushed the pr/auto-focus-calendar-input branch from a2c2100 to 39a0617 Compare December 25, 2018 01:39
@onlyann
Copy link
Contributor Author

onlyann commented Dec 25, 2018

@zombieJ I added a focusablePanel flag to the Calendar instead.
It's been enabled on one of the antd-calendar examples.
Let me know if something else needs to be done.

🎄 🎁

@zombieJ
Copy link
Member

zombieJ commented Dec 26, 2018

wow! That's wonderful!
Could we make it as default behavior instead of additional prop focusablePanel?

@onlyann
Copy link
Contributor Author

onlyann commented Dec 26, 2018

@zombieJ I added a default of true to focusablePanel.

I personally find it useful to expose it as a prop so that it can be switched to false, effectively making it a single Tab key stroke to move in and out of the Calendar component.

@zombieJ
Copy link
Member

zombieJ commented Jan 1, 2019

OPS. Conflict since this PR: #470

@zombieJ zombieJ merged commit e3b543a into react-component:master Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants