close the calendar on input blur #485

mnich0ls · 2018-05-09T23:18:55Z

Change the calendar close implementation.
Remove document click handler (which may fail if click propagation is stopped by another handler).
Use input blur event instead.
Use @mousedown.prevent on calendar containers to prevent change in focus from the input to the calendar.

Closes #484

coveralls · 2018-05-09T23:22:18Z

Coverage increased (+0.3%) to 100.0% when pulling e0fd0fc on mnich0ls:close-calendar-bug-fix into 3f568ff on charliekassel:master.

Change the calendar close implementation. Remove document click handler (which may fail if click propagation is stopped by another handler). Use input blur event instead. Use @mousedown.prevent on calendar containers to prevent change in focus from the input to the calendar.

charliekassel

This is really nice @mnich0ls - I didn't know about using mousedown preventDefault to keep the focus on the input.

I've just left a minor comment about my preference for guards and there's just one failing test - more than happy to merge once those are addressed.

charliekassel · 2018-05-10T06:56:20Z

test/unit/specs/DateInput/typedDates.spec.js

@@ -32,19 +32,17 @@ describe('DateInput', () => {
  it('emits the date when typed', () => {
    const input = wrapper.find('input')
    wrapper.vm.input.value = '2018-04-24'
+    input.trigger('keypress')


Seems like this line makes the test fail

hmm, I'm getting different tests results locally. This passes for me and I added this line because my tests results were showing that line 103 of DateInput.vue was uncovered. Any pointers how ensuring I get the same results as travis-ci?

charliekassel · 2018-05-10T07:00:21Z

src/components/DateInput.vue

-      }
-      if (isNaN(Date.parse(this.input.value))) {
-        this.clearDate()
+      if (this.typedDate && this.typeable) {


I'd prefer to keep the nesting level lower and return if we don't pass this.
I find this style much easier to read and reason about.

Additionally typedDate should never be truthy if typeable is false.

charliekassel · 2018-05-10T07:01:04Z

src/components/DateInput.vue

-      if (!isNaN(typedDate)) {
-        this.typedDate = this.input.value
-        this.$emit('typedDate', new Date(this.typedDate))
+      if (this.typeable) {


If this is false, I'd prefer to return here.

charliekassel · 2018-05-10T07:01:56Z

src/components/PickerDay.vue

@@ -1,5 +1,5 @@
 <template>
-  <div :class="[calendarClass, 'vdp-datepicker__calendar']" v-show="showDayView" :style="calendarStyle">
+  <div :class="[calendarClass, 'vdp-datepicker__calendar']" v-show="showDayView" :style="calendarStyle" @mousedown.prevent>


charliekassel · 2018-05-10T07:02:40Z

test/unit/specs/Datepicker/Datepicker.spec.js

@@ -43,7 +43,7 @@ describe('Datepicker mounted', () => {
    wrapper.vm.setValue()
    expect(wrapper.vm.selectedDate).toEqual(null)
    const pageDate = new Date(wrapper.vm.pageDate)
-    expect(pageDate.getYear()).toEqual(now.getYear())
+    expect(pageDate.getFullYear()).toEqual(now.getFullYear())


👍 getYear is so wonky :/

simplify nested if statement in inputBlurred()

mnich0ls · 2018-05-10T16:52:53Z

Thanks for reviewing! I pushed the changes minus the failing test. I can just remove the line that seems to be causing travis-ci to fail? I wish I could reproduce it locally.

remove call to trigger keypress on input that causes test to fail.

mnich0ls · 2018-05-10T16:56:58Z

Here's what I get locally when I remove input.trigger('keypress') (ah, I see travis gets the same result. The test doesn't fail for me locally and I don't get the warning about the uncovered line with input.trigger('keypress'))

charliekassel · 2018-05-11T06:08:42Z

Weirdly if you trigger the keypress to after the keyup it works...
I think we can add this unit test to cover that line

it('allows typing', () => {
    wrapper.setProps({typeable: true})
    expect(wrapper.vm.allowTyping()).toEqual(true)
    wrapper.setProps({typeable: false})
    expect(wrapper.vm.allowTyping({preventDefault: () => {}})).toEqual(false)
  })

complete code coverage of DateInput with additional test in typedDates.spec.js to ensure input allows keyboard input when configured accordingly.

mnich0ls · 2018-05-11T14:01:37Z

Thanks! Added the additional test to complete test coverage.

charliekassel · 2018-05-11T14:33:55Z

Good job @mnich0ls thanks!

charliekassel · 2018-05-11T18:19:52Z

Available as a minor bump in v1.2.0

mnich0ls · 2018-05-14T14:58:39Z

👍

mnich0ls force-pushed the close-calendar-bug-fix branch from 10b01bc to d54c916 Compare May 9, 2018 23:19

mnich0ls changed the title ~~close the calendar in input blur~~ close the calendar on input blur May 9, 2018

mnich0ls force-pushed the close-calendar-bug-fix branch from d54c916 to 251d865 Compare May 9, 2018 23:24

mnich0ls force-pushed the close-calendar-bug-fix branch from 5d851e1 to 630c08d Compare May 10, 2018 01:05

charliekassel reviewed May 10, 2018

View reviewed changes

remove nested if statement from DateInput.vue

6f0da54

simplify nested if statement in inputBlurred()

fix failing test

ea4caad

remove call to trigger keypress on input that causes test to fail.

add test to typedDates.spec.js

e0fd0fc

complete code coverage of DateInput with additional test in typedDates.spec.js to ensure input allows keyboard input when configured accordingly.

charliekassel merged commit 1704388 into charliekassel:master May 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

close the calendar on input blur #485

close the calendar on input blur #485

mnich0ls commented May 9, 2018

coveralls commented May 9, 2018 •

edited

charliekassel left a comment

charliekassel May 10, 2018

mnich0ls May 10, 2018

charliekassel May 10, 2018

mnich0ls May 10, 2018

charliekassel May 10, 2018

charliekassel May 10, 2018

charliekassel May 10, 2018

mnich0ls commented May 10, 2018

mnich0ls commented May 10, 2018 •

edited

charliekassel commented May 11, 2018

mnich0ls commented May 11, 2018

charliekassel commented May 11, 2018

charliekassel commented May 11, 2018

mnich0ls commented May 14, 2018

close the calendar on input blur #485

close the calendar on input blur #485

Conversation

mnich0ls commented May 9, 2018

coveralls commented May 9, 2018 • edited

charliekassel left a comment

Choose a reason for hiding this comment

charliekassel May 10, 2018

Choose a reason for hiding this comment

mnich0ls May 10, 2018

Choose a reason for hiding this comment

charliekassel May 10, 2018

Choose a reason for hiding this comment

mnich0ls May 10, 2018

Choose a reason for hiding this comment

charliekassel May 10, 2018

Choose a reason for hiding this comment

charliekassel May 10, 2018

Choose a reason for hiding this comment

charliekassel May 10, 2018

Choose a reason for hiding this comment

mnich0ls commented May 10, 2018

mnich0ls commented May 10, 2018 • edited

charliekassel commented May 11, 2018

mnich0ls commented May 11, 2018

charliekassel commented May 11, 2018

charliekassel commented May 11, 2018

mnich0ls commented May 14, 2018

coveralls commented May 9, 2018 •

edited

mnich0ls commented May 10, 2018 •

edited