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

feat(datepicker): esc can close datepicker #3966

Merged
merged 4 commits into from
Dec 13, 2018

Conversation

5earle
Copy link
Contributor

@5earle 5earle commented Mar 11, 2018

Closes #3890

PR Checklist

Before creating new PR, please take a look at checklist below to make sure that you've done everything that needs to be done before we can merge it.

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated tests.
  • added/updated API documentation.
  • added/updated demos.

@codecov
Copy link

codecov bot commented Mar 11, 2018

Codecov Report

Merging #3966 into development will decrease coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #3966      +/-   ##
===============================================
- Coverage        74.86%   74.83%   -0.03%     
===============================================
  Files              276      276              
  Lines             8399     8422      +23     
  Branches          1596     1601       +5     
===============================================
+ Hits              6288     6303      +15     
- Misses            1668     1673       +5     
- Partials           443      446       +3
Impacted Files Coverage Δ
...c/datepicker/bs-daterangepicker-input.directive.ts 25.3% <0%> (-0.31%) ⬇️
src/datepicker/bs-datepicker-input.directive.ts 45.2% <0%> (-0.63%) ⬇️
src/utils/public_api.ts 100% <100%> (ø) ⬆️
src/datepicker/bs-datepicker.component.ts 64.1% <100%> (+0.46%) ⬆️
src/component-loader/component-loader.class.ts 77.08% <100%> (+0.82%) ⬆️
src/datepicker/bs-daterangepicker.component.ts 38.66% <100%> (+0.82%) ⬆️
src/utils/triggers.ts 66.19% <45.45%> (-3.81%) ⬇️

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 92efe9a...318a226. Read the comment docs.

@5earle
Copy link
Contributor Author

5earle commented Mar 11, 2018

Addresses #3890

Copy link
Member

@valorkin valorkin left a comment

Choose a reason for hiding this comment

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

Hey, thanks for PR!
I think we could merge close on ESC
but not sure about tab related logic

if you could split this PR into 2 (one with ESC one with Tab, logic) it would be easier to merge

@@ -298,6 +301,16 @@ export class ComponentLoader<T> {
});
});
}
if (this._listenOpts.outsideEsc) {
const target = this._componentRef.location.nativeElement;
setTimeout(() => {

This comment was marked as off-topic.

@valorkin valorkin modified the milestones: 3.1.3, 3.1.4 Dec 3, 2018
@Domainv Domainv added this to the 3.1.4 milestone Dec 7, 2018
@ludmilanesvitiy ludmilanesvitiy changed the title tab can close datpicker, esc can exit datpicker tab can close datepicker, esc can exit datepicker Dec 12, 2018
@Domainv Domainv changed the title tab can close datepicker, esc can exit datepicker feat(datepicker): tab can close datepicker, esc can close datepicker Dec 12, 2018
@ghost ghost assigned Domainv Dec 12, 2018
@ghost ghost added needs review and removed needs fix labels Dec 12, 2018
@Domainv Domainv changed the title feat(datepicker): tab can close datepicker, esc can close datepicker feat(datepicker): esc can close datepicker Dec 13, 2018
@Domainv Domainv force-pushed the tab-closes-esc-exit branch from 6a9bf95 to c00b348 Compare December 13, 2018 09:30
@Domainv
Copy link
Contributor

Domainv commented Dec 13, 2018

'tab can close datepicker' functionality was cut out as unnecessary for now

Domainv
Domainv previously approved these changes Dec 13, 2018
@ludmilanesvitiy
Copy link
Contributor

Smoke tested in Chrome, FF -Esc always close the Datepicker. Looks good.

Copy link
Member

@valorkin valorkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks ;)

@valorkin valorkin merged commit 3ee6eac into valor-software:development Dec 13, 2018
@ghost ghost removed the ready for merge label Dec 13, 2018
leo6104 pushed a commit to leo6104/ngx-bootstrap that referenced this pull request Oct 10, 2019
Fixes valor-software#3890
* tab can close datpicker, esc can exit datpicker

* fix(datepicker): conflicts resolve & leave only esc datepicker close

* feat(datepicker): add test
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.

4 participants