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): close date-picker after tabbing out #6571

Merged
merged 5 commits into from
Apr 6, 2021

Conversation

KeelanS
Copy link
Contributor

@KeelanS KeelanS commented Apr 1, 2021

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?

Issue Number: 5844

What is the new behavior?

When tabbing out of the datepicker the picker will close.
When navigating in a range picker, the picker will only close if you go outside of the datepicker. Tabbing between the left and right side of the range picker will not close the picker.

This works both while tabbing or shift-tabbing.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@codecov
Copy link

codecov bot commented Apr 1, 2021

Codecov Report

Merging #6571 (dbc2d65) into master (bc0e468) will increase coverage by 0.03%.
The diff coverage is 94.11%.

❗ Current head dbc2d65 differs from pull request most recent head d9a31b8. Consider uploading reports for the commit d9a31b8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6571      +/-   ##
==========================================
+ Coverage   89.88%   89.92%   +0.03%     
==========================================
  Files         481      481              
  Lines       15374    15375       +1     
  Branches     2343     2344       +1     
==========================================
+ Hits        13819    13826       +7     
+ Misses        937      929       -8     
- Partials      618      620       +2     
Impacted Files Coverage Δ
components/select/select.component.ts 87.09% <66.66%> (ø)
components/date-picker/date-picker.component.ts 93.50% <100.00%> (+0.04%) ⬆️
components/date-picker/picker.component.ts 89.94% <100.00%> (ø)
components/select/option-group.component.ts 100.00% <100.00%> (ø)
components/select/option-item-group.component.ts 100.00% <100.00%> (ø)
components/select/option-item.component.ts 93.10% <100.00%> (ø)
components/select/option.component.ts 100.00% <100.00%> (ø)
components/select/select-item.component.ts 93.33% <100.00%> (ø)
components/space/space.component.ts 100.00% <100.00%> (ø)
components/table/src/table-data.service.ts 84.61% <100.00%> (ø)
... and 6 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 bc0e468...d9a31b8. Read the comment docs.

@KeelanS
Copy link
Contributor Author

KeelanS commented Apr 1, 2021

This fix worked out of the box for all examples on the demo page except for the 'start-end' one where you create a custom range picker. Pressing tab while in the left input would open the right picker, but wouldn't select the input itself which meant that you couldn't write your date.

Since it was a custom implementation I adapted it to work with my fix. This did remove the usage of the ngOnOpenChange outputs. If this is a problem I could change it back and try to fix it another way. But I have tried doing that and it's not that obvious.

Anyway I'd appreciate any input on this :)

@wenqi73
Copy link
Member

wenqi73 commented Apr 2, 2021

@KeelanS Should not just use tab as the judgment. After clicking tab, just see the active element is in the input, if not, close the panel, and the demo will not be affected. BTW, add a test for range picker, that will be better.

@KeelanS
Copy link
Contributor Author

KeelanS commented Apr 2, 2021

@wenqi73 You mean just on the 'start-end' demo component or everywhere?

Right now this component works like I would think it should work. One assumption I made was that after a 'tab', the focus would go on the other input without the picker opening.

I guess this example is still OK since it still shows you can use the 'open' method and the 'nzDisabledDate' to customize your datepickers. And that was the main purpose of this example.

if (!this.isRange && event.key === 'Tab') {
this.close();
} else if (this.isRange && event.key === 'Tab') {
if (this.datePickerService.activeInput === 'left' && event.shiftKey) {
Copy link
Member

Choose a reason for hiding this comment

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

The tab action may be changed in the future. I think this way is better:

if (!this.elementRef.nativeElement.contains(document.activeElement)) { 
  // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to check on focusEvent since at both keyDown and keyUp, the nativeElement and activeElement would be the same.

@@ -5,6 +5,7 @@ import { NzDatePickerComponent } from 'ng-zorro-antd/date-picker';
selector: 'nz-demo-date-picker-start-end',
template: `
<nz-date-picker
#startDatePicker
Copy link
Member

Choose a reason for hiding this comment

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

Useless code

Copy link
Member

@wenqi73 wenqi73 left a comment

Choose a reason for hiding this comment

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

LGTM

@wenqi73 wenqi73 merged commit 21ded3f into NG-ZORRO:master Apr 6, 2021
@wenqi73
Copy link
Member

wenqi73 commented Apr 6, 2021

@KeelanS Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants