Choose minutes #119

CasperLaiTW · 2017-02-03T02:59:57Z

joews

This patch looks great, thanks! I have a few comments about code consistency, but nothing big to change 👍

joews · 2017-02-03T22:18:25Z

lib/js/index.js

@@ -126,12 +126,21 @@ class DateTimePicker extends Events {
    this.$('.js-ok')
      .addEventListener('click', () => this.clickSubmit(), false);

+    this.$('.c-datepicker__header-date__hours').addEventListener('click', e => this.showHourClock(e), false);


Please use .js-date-hours here - BEM classnames are for style, js- classes are for JavaScript. I know we don't totally adhere to this yet, but that's the aim 😄

The idea comes from this article by Harry Roberts, btw - it's a good read!

joews · 2017-02-03T22:19:07Z

lib/js/index.js

@@ -126,12 +126,21 @@ class DateTimePicker extends Events {
    this.$('.js-ok')
      .addEventListener('click', () => this.clickSubmit(), false);

+    this.$('.c-datepicker__header-date__hours').addEventListener('click', e => this.showHourClock(e), false);
+    this.$('.c-datepicker__header-date__minutes').addEventListener('click', e => this.showMinuteClock(e), false);


.js-date-minutes, please

joews · 2017-02-03T22:20:02Z

lib/js/index.js

    this.$('.c-datepicker__clock__hours').addEventListener('mouseleave', e => this.mouseOutClock(e), false);
-    this.$(`.${this.options.styles.clockNum}`).forEach((el) => {
+    this.$(`.c-datepicker__clock__hours .${this.options.styles.clockNum}`).forEach((el) => {


Can you add a .js- class for this the hours element?

joews · 2017-02-03T22:21:46Z

lib/js/index.js

      el.addEventListener('click', e => this.clickClock(e), false);
      el.addEventListener('mouseenter', e => this.mouseInClock(e), false);
    });

+    this.$('.c-datepicker__clock__minutes').addEventListener('mouseleave', e => this.mouseOutClock(e), false);


.js- class please. I'll stop making this comment now, but it should apply to each new className referenced in JavaScript.

I'm going to do the same to any pre-existing non js- classes soon.

joews · 2017-02-03T22:25:28Z

lib/js/index.js

@@ -171,6 +180,13 @@ class DateTimePicker extends Events {
    return this;
  }

+  clickClockMinutes(e) {


(Github won't let me add a comment to the right line) - do you need to change clickClock to clickClickHour?

I changed clickClock recently to call this.set (to fix #110) - can you update this method to follow the same pattern?

joews · 2017-02-03T22:28:48Z

lib/js/index.js

@@ -216,6 +240,20 @@ class DateTimePicker extends Events {
    return this;
  }

+  showHourClock() {
+    this.$('.c-datepicker__clock__hours').style.display = 'block';


It seems inconsistent to use style.display for one part of the UI and a className for the other. Could you use an active class for both?

joews · 2017-02-03T22:36:37Z

This is awesome, thank you! One usability comment - the hour and minute have cursor: pointer when the calendar is active. They are not clickable in this state.

Two possible fixes:

Only use cursor: pointer when clock is active
When the calendar is active, clicking hours/minutes opens the clock

The second option would be cool, but maybe it's one for another PR. If we do this we should also allow clicking on the date to open the calendar.

What do you think?

CasperLaiTW · 2017-02-04T06:28:02Z

I fix requested changes :)

About click to open clock, I make first option for this PR.

And I think I can keep working for second option include clicking on the date to open the calendar

joews

Thanks for the changes! I found a few more consistency issues that I missed last time, and a bug that occurs when a default time is set. Once those are fixed I'll merge 👍

joews · 2017-02-05T13:05:35Z

lib/js/index.js

+
+    this.$('.js-clock-hours').addEventListener('mouseleave', e => this.mouseOutClock(e), false);
+    this.$(`.js-clock-hours .${this.options.styles.clockNum}`).forEach((el) => {
+      el.addEventListener('click', e => this.clickClickHour(e), false);
      el.addEventListener('mouseenter', e => this.mouseInClock(e), false);


I missed this before - can you change to mouseInClockHour ?

do you mean mouseInClock change to mouseInHourClock ?

Yes, that seems a better name. Thanks!

I make changes, can you check it?

joews · 2017-02-05T13:06:05Z

lib/js/index.js

+      active.classList.add('hide-hand');
+    }
+  }
+
  mouseOutClock() {


Can you make this mouseOutHourClock ?

joews · 2017-02-05T13:07:06Z

lib/js/index.js

    this.$(`.c-datepicker__clock__hours .${this.options.styles.clockNum}[data-number="${hourAsInt}"]`)
      .classList.add(`${this.options.styles.clockNum}--active`);
+    this.$(`.c-datepicker__clock__minutes .${this.options.styles.clockNum}[data-number="${minuteAsInt}"]`)


I found a bug in testing - if the minutes from the datepicker's default time are not on the clock faces, this throws:

index.js:353 Uncaught TypeError: Cannot read property 'classList' of undefined at DateTimePicker.setTime (index.js:353) at DateTimePicker.open (index.js:92) at HTMLAnchorElement.<anonymous>

You can test this by adding default: moment() to the Datepicker options. If the current minutes is not a multiple of 5, you'll hit this error.

joews · 2017-02-05T13:08:44Z

lib/js/index.js

@@ -188,8 +206,16 @@ class DateTimePicker extends Events {
    }
  }

+  mouseInMinuteClock() {
+    const active = this.$(`.c-datepicker__clock__minutes .${this.options.styles.clockNum}--active`);


Can you use .js-clock-minutes instead of .c-datepicker__clock__minutes

joews · 2017-02-05T13:09:18Z

lib/js/index.js

@@ -216,6 +242,20 @@ class DateTimePicker extends Events {
    return this;
  }

+  showHourClock() {
+    this.$('.c-datepicker__clock__hours').classList.add('active');


.js-clock-hours

joews · 2017-02-05T13:10:06Z

lib/js/index.js

+  showHourClock() {
+    this.$('.c-datepicker__clock__hours').classList.add('active');
+    this.$('.c-datepicker__clock__minutes').classList.remove('active');
+    this.$('.c-datepicker__header-date__hours').classList.add('active');


.js-date-hours

joews · 2017-02-05T13:10:14Z

lib/js/index.js

+    this.$('.c-datepicker__clock__hours').classList.add('active');
+    this.$('.c-datepicker__clock__minutes').classList.remove('active');
+    this.$('.c-datepicker__header-date__hours').classList.add('active');
+    this.$('.c-datepicker__header-date__minutes').classList.remove('active');


.js-date-minutes

joews · 2017-02-05T13:10:46Z

lib/js/index.js

+  }
+
+  showMinuteClock() {
+    this.$('.c-datepicker__clock__hours').classList.remove('active');


The same className changes as above, please

joews · 2017-02-05T13:11:48Z

lib/js/index.js

    const hourAsInt = parseInt(hour, 10) % 12;
+    const minuteAsInt = parseInt(minutes, 10);
    const oldActiveHours = this.$(`.c-datepicker__clock__hours .${this.options.styles.clockNum}--active`);


.js-clock-hours, please - this one was here already but this seems like a good time to improve the code quality while making similar changes 😄

joews · 2017-02-05T13:11:58Z

lib/js/index.js

    const oldActiveHours = this.$(`.c-datepicker__clock__hours .${this.options.styles.clockNum}--active`);
+    const oldActiveMinutes = this.$(`.c-datepicker__clock__minutes .${this.options.styles.clockNum}--active`);


.js-clock-minutes

CasperLaiTW · 2017-02-05T14:49:16Z

I make changes
About bug I make it will go next multiple of 5

joews

LGTM! thanks for your patience with the reviews 😄

joews · 2017-02-05T22:17:32Z

Thanks for the contribution, this is such an important feature! Released as v2.1.0.

joews requested changes Feb 3, 2017

View reviewed changes

joews mentioned this pull request Feb 3, 2017

Allow to choose minutes #87

Closed

CasperLaiTW force-pushed the choose-minutes-patch branch from 264423a to 66929a5 Compare February 4, 2017 06:25

joews requested changes Feb 5, 2017

View reviewed changes

Choose minutes

db042f0

CasperLaiTW force-pushed the choose-minutes-patch branch from 66929a5 to db042f0 Compare February 5, 2017 14:47

joews approved these changes Feb 5, 2017

View reviewed changes

joews merged commit 011b0b4 into ripjar:master Feb 5, 2017

joews mentioned this pull request Feb 5, 2017

How do you select minutes? #72

Closed

joews mentioned this pull request Feb 9, 2017

set always rounds time up to the next 5 minute interval; it should round to the nearest #124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Choose minutes #119

Choose minutes #119

CasperLaiTW commented Feb 3, 2017

joews left a comment

joews Feb 3, 2017

joews Feb 3, 2017

joews Feb 3, 2017

joews Feb 3, 2017

joews Feb 3, 2017

joews Feb 3, 2017

joews commented Feb 3, 2017

CasperLaiTW commented Feb 4, 2017

joews left a comment

joews Feb 5, 2017

CasperLaiTW Feb 5, 2017

joews Feb 5, 2017

CasperLaiTW Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

joews Feb 5, 2017

CasperLaiTW commented Feb 5, 2017

joews left a comment

joews commented Feb 5, 2017

		const oldActiveHours = this.$(`.c-datepicker__clock__hours .${this.options.styles.clockNum}--active`);
		const oldActiveMinutes = this.$(`.c-datepicker__clock__minutes .${this.options.styles.clockNum}--active`);

Choose minutes #119

Choose minutes #119

Conversation

CasperLaiTW commented Feb 3, 2017

joews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joews commented Feb 3, 2017

CasperLaiTW commented Feb 4, 2017

joews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CasperLaiTW commented Feb 5, 2017

joews left a comment

Choose a reason for hiding this comment

joews commented Feb 5, 2017