feat: add API for range setting #61

jajugoguma · 2021-05-31T12:39:56Z

Please check if the PR fulfills these requirements

It's submitted to right branch according to our branching model
It's right issue type on title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has description for the breaking change

Description

add range setting API

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

lja1018

리뷰완료합니다.

lja1018 · 2021-06-01T06:12:54Z

examples/example05-using-range.html

+<!DOCTYPE html>
+    <head lang="en">
+        <meta charset="UTF-8">
+        <title>3. Using meridiem</title>


안바꼈네요

lja1018 · 2021-06-01T06:17:31Z

src/js/timepicker/index.js

+      var disabledItems;
+      disabledItems = this._disabledMinutes[hour] ? this._disabledMinutes[hour] : [];


var disabledItems = this._disabledMonutes[hour] || [];로 해도 될 것 같습니다.

lja1018 · 2021-06-01T06:19:12Z

src/js/timepicker/index.js

+
+      disabledHours = util.getRangeArr(0, beginHour - 1);
+      disabledMinRanges.push({
+        begin: 0,


0, 59는 상수로 빼도 될 것 같아요.

23이나 12도 마찬가지입니다!

lja1018 · 2021-06-01T06:20:51Z

src/js/timepicker/index.js

+      if (hour < 0 || hour > 23) {
+        return false;
+      }
+
+      if (minute < 0 || minute > 59) {


0, 23, 59도 상수처리하는게 좋을 것 같습니다.

jung-han · 2021-06-01T06:38:40Z

src/js/util.js

+    for (i = 0; i < 60; i += 1) {
+      arr.push(false);
+    }


fill 유틸 메서드 하나 만들어서 사용하면 좋겠네요 ㅎ

jung-han · 2021-06-01T06:38:56Z

src/js/util.js

+    forEachArray(enableRanges, function(enableRange) {
+      for (i = enableRange.begin; i <= enableRange.end; i += 1) {
+        arr[i] = true;
+      }
+    });


그럼 여기도 같이 사용할 수 있을것 같아요.

jung-han · 2021-06-01T06:39:26Z

src/js/timepicker/index.js

+     * @returns {boolean} result of range validation
+     * @private
+     */
+    _CompareTimes: function(begin, end) {


저희 언더바 프리픽스 사용하지 않습니다!

메서드명이 대문자로 시작하네요

jung-han · 2021-06-01T06:40:33Z

src/js/timepicker/index.js

+      if (hour < 0 || hour > 23) {
+        return false;
+      }
+
+      if (minute < 0 || minute > 59) {
+        return false;
+      }


어려운 로직이 아니여서 그냥 한번에 분기문을 작성해도 괜찮을 것 같아요.

jung-han · 2021-06-01T06:41:03Z

src/js/timepicker/index.js

+      if (!this._isValidTime(endHour, endMin)) {
+        return false;
+      }
+
+      if (this._CompareTimes(begin, end) <= 0) {
+        return false;
+      }
+
+      return true;


이 부분도 한번에 줄여서 반환해줄수 있겠네요

jung-han · 2021-06-01T06:44:05Z

src/js/timepicker/index.js

+      var endHour;
+      var endMin;
+
+      var disabledHours;


var endHour, endMin, disabledHours로 바로 작성해도 되겠네요

jung-han · 2021-06-01T06:44:40Z

src/js/timepicker/index.js

+      if (end) {
+        endHour = end.hour;
+        endMin = end.minute;
+        disabledHours = disabledHours.concat(util.getRangeArr(endHour + 1, 23));
+
+        disabledMinRanges.push({
+          begin: endMin,
+          end: 59
+        });
+      }
+
+      if (disabledMinRanges.length > 1 && beginHour === endHour) {
+        this._disabledMinutes[beginHour] = util.getDisabledMinuteArr(disabledMinRanges).concat();
+      } else {
+        this._disabledMinutes[beginHour] = util.getDisabledMinuteArr([disabledMinRanges[0]]).concat();
+        this._disabledMinutes[endHour] = util.getDisabledMinuteArr([disabledMinRanges[1]]).concat();
+      }
+
+      this._disabledHours = disabledHours.concat();
+
+      this.setTime(beginHour, beginMin);
+      this._setDisabledHours();
+
+      if (this._showMeridiem) {
+        this._syncToMeridiemElements();
+
+        util.setDisabled(this._amEl, beginHour >= 12);
+        util.setDisabled(this._pmEl, endHour < 12);


이 메서드에서 너무 많은 일을 하고 있는것 같은데 조금 메서드를 분리해서 작성해보는건 어때요?

jung-han

완료입니다

jungeun-cho · 2021-06-01T07:16:42Z

src/js/timepicker/index.js

@@ -510,6 +522,13 @@ var TimePicker = defineClass(
      this._hourInput.setDisabledItems(disabledItems);
    },

+    _setDisabledMinutes: function(hour) {
+      var disabledItems;
+      disabledItems = this._disabledMinutes[hour] ? this._disabledMinutes[hour] : [];


this._disabledMinutes[hour] ?? []

jungeun-cho · 2021-06-01T07:20:12Z

src/js/timepicker/index.js

+      if (!this._isValidTime(endHour, endMin)) {
+        return false;
+      }
+
+      if (this._CompareTimes(begin, end) <= 0) {
+        return false;
+      }


두개 조건 합쳐서 if문만들수있어요

jungeun-cho

리뷰완료합니다.
배포전 $ npm run doc:dev 돌려서 doc한번확인해주세요~

shiren

수고하셨어요

shiren · 2021-06-03T05:33:18Z

src/js/timepicker/index.js

+      }
+
+      return true;
+    },


첫번째 탈출 조건을 반대로 해서 리턴하면 리턴문 하나로 쓸 수 있지 않을까요?

shiren · 2021-06-03T05:33:56Z

src/js/timepicker/index.js

+        = this.element
+        = this.meridiemElement
+        = this.amEl
+        = this.pmEl


ㅋㅋ 이 표기법은 뭔가 이쁘면서도 오묘하게 거슬리네 ㅋㅋ

El과 Element와 element가 모두 공존해서 그런가보네요 ㅋㅋ

shiren · 2021-06-03T05:34:41Z

src/js/util.js

+   * @returns {Array}
+   */
+  fill: function(start, end, value, target) {
+    var arr = target || [];


디폴트 파라메터를 쓰면 될 것 같아요.

lja1018

리뷰완료합니다.

lja1018 · 2021-06-03T06:15:56Z

src/js/timepicker/index.js

+      if (!this.isValidTime(endHour, endMin) || this.compareTimes(begin, end) <= 0) {
+        return false;
+      }
+
+      return true;


이 부분은 return (this.isValidTime(endHour, endMin) && this.compareTimes(begin, end) > 0);으로 바꿀 수 있을 거 같아요.

lja1018 · 2021-06-03T06:19:06Z

src/js/util.js

+      arr[i] = value;
+    }
+
+    for (; i <= end; i += 1) {


i가 replaceEnd부터 시작하는데 시작 조건에 명시적으로 적어줘도 좋을 것 같습니다.

jung-han

완료입니다~

jungeun-cho · 2021-06-03T09:09:11Z

확인했습니다~

jajugoguma added 3 commits May 31, 2021 21:19

test: add test for setting range

d278d1e

feat: add selectable range setting

f2164c8

chore: add example of using range

96451e1

jajugoguma requested review from heenakwag and lja1018 May 31, 2021 12:39

lja1018 reviewed Jun 1, 2021

View reviewed changes

jung-han reviewed Jun 1, 2021

View reviewed changes

jung-han approved these changes Jun 1, 2021

View reviewed changes

jungeun-cho reviewed Jun 1, 2021

View reviewed changes

jungeun-cho approved these changes Jun 1, 2021

View reviewed changes

chore: apply reviews

c498981

shiren reviewed Jun 3, 2021

View reviewed changes

lja1018 approved these changes Jun 3, 2021

View reviewed changes

jung-han approved these changes Jun 3, 2021

View reviewed changes

chore: apply code reviews

cb9672f

jajugoguma merged commit 34b6f8e into master Jun 4, 2021

jajugoguma deleted the feat/set-range branch June 4, 2021 01:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add API for range setting #61

feat: add API for range setting #61

jajugoguma commented May 31, 2021

lja1018 left a comment

lja1018 Jun 1, 2021

lja1018 Jun 1, 2021

lja1018 Jun 1, 2021

lja1018 Jun 1, 2021

lja1018 Jun 1, 2021

jung-han Jun 1, 2021

jung-han Jun 1, 2021

jung-han Jun 1, 2021

jung-han Jun 1, 2021

jung-han Jun 1, 2021

jung-han Jun 1, 2021

jung-han Jun 1, 2021

jung-han Jun 1, 2021

jung-han left a comment

jungeun-cho Jun 1, 2021

jungeun-cho Jun 1, 2021

jungeun-cho left a comment

shiren left a comment

shiren Jun 3, 2021

shiren Jun 3, 2021

jung-han Jun 3, 2021

shiren Jun 3, 2021 •

edited

Loading

lja1018 left a comment

lja1018 Jun 3, 2021

lja1018 Jun 3, 2021

jung-han left a comment

jungeun-cho commented Jun 3, 2021

		var disabledItems;
		disabledItems = this._disabledMinutes[hour] ? this._disabledMinutes[hour] : [];

feat: add API for range setting #61

feat: add API for range setting #61

Conversation

jajugoguma commented May 31, 2021

Please check if the PR fulfills these requirements

Description

lja1018 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

jung-han left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungeun-cho left a comment

Choose a reason for hiding this comment

shiren 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

shiren Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

lja1018 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jung-han left a comment

Choose a reason for hiding this comment

jungeun-cho commented Jun 3, 2021

shiren Jun 3, 2021 •

edited

Loading