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

fix(module:date-picker): open on enter and focus on inner input #3804

Conversation

@jimmytheneutrino
Copy link
Contributor

commented Jul 16, 2019

Extracted the good parts suitable for a11y from #3146. See discussion there.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Opening date-picker with keyboard is not possible.

Opening date-picker by a click, does not focus on the inner input element.

Issue Number: N/A

What is the new behavior?

You can open date-picker pressing enter on the keyboard when the wrapper element is selected.

Once date-picker is open, the inner input element is selected, so it is immediately possible to input date from keyboard. (Of course, this is only applicable to the cases, when there is an inner input element: e.g., month and year pickers do not have any).

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@netlify

This comment has been minimized.

Copy link

commented Jul 16, 2019

Deploy preview for ng-zorro-master ready!

Built with commit 4db785f

https://deploy-preview-3804--ng-zorro-master.netlify.com

@wendzhue wendzhue requested a review from wenqi73 Jul 16, 2019

@@ -135,6 +139,7 @@ export class NzPickerComponent implements OnInit, AfterViewInit {
if (this.realOpenState) {
this.overlayOpen = false;
this.openChange.emit(this.overlayOpen);
this.focus();

This comment has been minimized.

Copy link
@wenqi73

wenqi73 Jul 16, 2019

Member

Should need a test for focus.

This comment has been minimized.

Copy link
@jimmytheneutrino

jimmytheneutrino Jul 16, 2019

Author Contributor

Added 'should focus on the trigger after a click outside' tests

@@ -33,6 +33,7 @@
[locale]="locale"
[disabledDate]="disabledDate"
[format]="format"
[autoFocus]="partType != 'right'"

This comment has been minimized.

Copy link
@wenqi73

wenqi73 Jul 16, 2019

Member
Suggested change
[autoFocus]="partType != 'right'"
[autoFocus]="partType !== 'right'"

This comment has been minimized.

Copy link
@jimmytheneutrino

jimmytheneutrino Jul 16, 2019

Author Contributor

fixed

@wenqi73

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Thank you @jimmytheneutrino, this will close #3530.

@codecov

This comment has been minimized.

Copy link

commented Jul 16, 2019

Codecov Report

Merging #3804 into master will increase coverage by <.01%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3804      +/-   ##
==========================================
+ Coverage   95.39%   95.39%   +<.01%     
==========================================
  Files         712      712              
  Lines       14633    14640       +7     
  Branches     1929     1930       +1     
==========================================
+ Hits        13959    13966       +7     
  Misses        244      244              
  Partials      430      430
Impacted Files Coverage Δ
components/date-picker/picker.component.ts 97.89% <100%> (+0.06%) ⬆️
...te-picker/lib/calendar/calendar-input.component.ts 94.44% <85.71%> (+0.69%) ⬆️

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 ab8a58c...f13c5eb. Read the comment docs.

@jimmytheneutrino jimmytheneutrino force-pushed the jimmytheneutrino:fix-datepicker-autofocus-and-onenter branch from 3b0ee51 to fa40ad5 Jul 16, 2019

@wendzhue
Copy link
Member

left a comment

Two problems:

  1. If you try to input a day, the panel would auto close when if you're done.
  2. If you reopen the date-picker, you can press enter to confirm your selection.

These are different from the React version.

@jimmytheneutrino jimmytheneutrino force-pushed the jimmytheneutrino:fix-datepicker-autofocus-and-onenter branch from fa40ad5 to b47440a Jul 18, 2019

@jimmytheneutrino

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@wendzhue
Of these 2 problems, 2 seemed to be easy to do and quite useful, so I included a fix and a test.

However, 1 seems to be more subtle. A probable fix (which I am not really aware of) requiring an enter on changes could possibly disrupt the behavior of different components, because calendar-input is used in many places. Besides, it is not really an improvement but conformance to the react version, which is not necessarily 100% better. Lastly, it is not really that connected to the PR at hand. I think it is better to state 1 in a separate issue.

@jimmytheneutrino jimmytheneutrino force-pushed the jimmytheneutrino:fix-datepicker-autofocus-and-onenter branch from b47440a to f13c5eb Jul 18, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 18, 2019

Codecov Report

Merging #3804 into master will decrease coverage by 0.01%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3804      +/-   ##
==========================================
- Coverage   95.25%   95.24%   -0.02%     
==========================================
  Files         726      726              
  Lines       14652    14679      +27     
  Branches     1907     1911       +4     
==========================================
+ Hits        13957    13981      +24     
- Misses        259      260       +1     
- Partials      436      438       +2
Impacted Files Coverage Δ
components/date-picker/picker.component.ts 97.89% <100%> (+0.06%) ⬆️
...te-picker/lib/calendar/calendar-input.component.ts 97.22% <85.71%> (+3.47%) ⬆️
components/tooltip/nz-tooltip.component.ts 88.88% <0%> (-1.59%) ⬇️
components/tabs/nz-tabs-nav.component.ts 89.88% <0%> (-1.08%) ⬇️
components/slider/nz-slider-marks.component.ts 89.74% <0%> (-0.74%) ⬇️
components/core/time/candy-date.ts 91.36% <0%> (-0.72%) ⬇️
components/upload/nz-upload.component.ts 99.28% <0%> (ø) ⬆️
components/upload/nz-upload-btn.component.ts 96.44% <0%> (ø) ⬆️
components/tabs/nz-tabs.module.ts 100% <0%> (ø) ⬆️
components/tree-select/nz-tree-select.component.ts 99.08% <0%> (+0.03%) ⬆️
... and 3 more

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 e9663a2...4db785f. Read the comment docs.

@jimmytheneutrino jimmytheneutrino force-pushed the jimmytheneutrino:fix-datepicker-autofocus-and-onenter branch from f13c5eb to 8d939cf Jul 19, 2019

@jimmytheneutrino

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@wendzhue
After the fix for 2, the codecov patch check failed no matter what. So it seemed easier to try a fix for 1 that would also probably eliminate codecov issues. Fixed the tests accordingly.

@wendzhue
Copy link
Member

left a comment

Please rebase to the master branch.

@jimmytheneutrino jimmytheneutrino force-pushed the jimmytheneutrino:fix-datepicker-autofocus-and-onenter branch from 8d939cf to 7694ba9 Jul 26, 2019

@jimmytheneutrino

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

It seems CI has failed with some strange 'Chromium disconnected' error. Rerun?

fix(module:date-picker): open on enter and focus on inner input
Extracted the good parts suitable for a11y from #3146. See discussion there.

@jimmytheneutrino jimmytheneutrino force-pushed the jimmytheneutrino:fix-datepicker-autofocus-and-onenter branch from 7694ba9 to 4db785f Jul 27, 2019

@jimmytheneutrino

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

The test job does not seem to be able to finish. (It is ok on my local machine.)
Please see, e.g., this thread for a similar mistake: karma-runner/karma-chrome-launcher#196

@wenqi73 wenqi73 merged commit 3f03ee1 into NG-ZORRO:master Aug 6, 2019

4 of 9 checks passed

codecov/patch 92.85% of diff hit (target 95.25%)
Details
codecov/project 95.24% (-0.02%) compared to e9663a2
Details
Header rules No header rules processed
Details
Pages changed 1 new file uploaded
Details
Redirect rules No redirect rules processed
Details
CodeFactor 4 issues fixed. 4 issues found.
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@pr-triage pr-triage bot added PR: merged and removed PR: reviewed-approved labels Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.