feat: set date to today by click today text(fix #87) #88

jajugoguma · 2021-07-29T08:41:20Z

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

Make today text of title look clickable and If click it, set the date to today on DatePicker.

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

dongsik-yoo

리뷰 완료합니다. 수고하셨습니다.

dongsik-yoo · 2021-07-29T10:32:19Z

src/js/datepicker/index.js

+     * @param {Event} ev An event object
+     * @private
+     */
+    _todayClickHandler: function() {


핸들러가 _on~~으로 시작해서 이것도 맞추는게 좋겠습니다.

_onClickTodayHandler 면 되겠네요

_onClickTodayHandler으로 핸들러 명 수정 했습니다.

dongsik-yoo · 2021-07-29T10:33:14Z

test/calendar/header.spec.js

@@ -11,6 +11,7 @@ var CLASS_NAME_NEXT_MONTH_BTN = constants.CLASS_NAME_NEXT_MONTH_BTN;
 var CLASS_NAME_NEXT_YEAR_BTN = constants.CLASS_NAME_NEXT_YEAR_BTN;
 var CLASS_NAME_PREV_MONTH_BTN = constants.CLASS_NAME_PREV_MONTH_BTN;
 var CLASS_NAME_PREV_YEAR_BTN = constants.CLASS_NAME_PREV_YEAR_BTN;
+var CLASS_NAME_TITLE_TODAY = 'tui-calendar-title-today';


위에는 변수로 선언된 것을 가져오는데, 이 부분은 다르게 한 이유가 있을까요?

위쪽에 변수로 선언된거에는 . 가 앞에 붙어있네요. 이런거는 어떤 형태로든 통일이 되도록 만드는게 좋겠는데 말이죠.

통일되도록 constants에서 가져오도록 변경했습니다.

dongsik-yoo · 2021-07-29T10:34:23Z

test/calendar/header.spec.js

+      header.on('today', spy);
+
+      clickBtnInHeader(CLASS_NAME_TITLE_TODAY);
+      expect(spy).toHaveBeenCalled();


오늘 날짜인지 데이터 비교도 테스트 해야 하지 않을까요?

Today라고 할 수 있는 시점을 모킹하고 임의로 날짜를 변경해본 다음 clickBtnInHeader 를 호출하고 난 뒤 Today 모킹이 출력되는지 보아야겠네요.

위 테스트는 헤더에서 오늘 날짜 텍스트를 클릭했을 때 today 이벤트가 정상적으로 발생하는지만 검증하는 테스트였습니다.
말씀하신 것을 검증하기 위해 데이트픽커에 대해 새 테스트를 작성했습니다.

dongsik-yoo · 2021-07-29T10:35:13Z

src/css/calendar.css

@@ -91,6 +91,12 @@
    background-color: #f4f4f4
 }

+.tui-calendar .tui-calendar-title-today:hover {
+    color: #333;
+    background-color: #edf4fc;


색상은 디자인 가이드에 있는 거예요?

hover 배경색이랑 동일한 것 같네요.

데이트 픽커에서 날짜 등을 선택하기 전 hover의 배경색과 동일하게 처리해 클릭 가능함을 알리려 위 색상을 사용했습니다.
혹시 이 버튼을 위한 색상 가이드를 다시 받아야할까요?

adhrinae

리뷰 완료합니다.

adhrinae · 2021-07-30T01:05:28Z

src/js/calendar/header.js

@@ -24,6 +24,7 @@ var TYPE_YEAR = constants.TYPE_YEAR;
 var CLASS_NAME_TITLE_MONTH = 'tui-calendar-title-month';
 var CLASS_NAME_TITLE_YEAR = 'tui-calendar-title-year';
 var CLASS_NAME_TITLE_YEAR_TO_YEAR = 'tui-calendar-title-year-to-year';
+var CLASS_NAME_TITLE_TODAY = '.tui-calendar-title-today';


위의 클래스네임은 . 가 없고 이 라인이랑 아래쪽에는 . 가 붙어있는 이유가 있나요?

단순히 클래스 네임이라 생각하고 저 그룹과 묶었었는데 실제 사용은 아래의 .이 붙어있는 그룹과 같이 사용되어서 이에 따른 오해가 발생하지 않도록 위치를 조정하고 이름은 변경했습니다.

adhrinae · 2021-07-30T01:06:19Z

src/js/datepicker/index.js

+     * @param {Event} ev An event object
+     * @private
+     */
+    _todayClickHandler: function() {


_onClickTodayHandler 면 되겠네요

adhrinae · 2021-07-30T01:07:04Z

test/calendar/header.spec.js

@@ -11,6 +11,7 @@ var CLASS_NAME_NEXT_MONTH_BTN = constants.CLASS_NAME_NEXT_MONTH_BTN;
 var CLASS_NAME_NEXT_YEAR_BTN = constants.CLASS_NAME_NEXT_YEAR_BTN;
 var CLASS_NAME_PREV_MONTH_BTN = constants.CLASS_NAME_PREV_MONTH_BTN;
 var CLASS_NAME_PREV_YEAR_BTN = constants.CLASS_NAME_PREV_YEAR_BTN;
+var CLASS_NAME_TITLE_TODAY = 'tui-calendar-title-today';


위쪽에 변수로 선언된거에는 . 가 앞에 붙어있네요. 이런거는 어떤 형태로든 통일이 되도록 만드는게 좋겠는데 말이죠.

adhrinae · 2021-07-30T01:08:34Z

test/calendar/header.spec.js

+      header.on('today', spy);
+
+      clickBtnInHeader(CLASS_NAME_TITLE_TODAY);
+      expect(spy).toHaveBeenCalled();


Today라고 할 수 있는 시점을 모킹하고 임의로 날짜를 변경해본 다음 clickBtnInHeader 를 호출하고 난 뒤 Today 모킹이 출력되는지 보아야겠네요.

lja1018

리뷰완료합니다.

jwlee1108

리뷰 완료합니다.

jajugoguma added 3 commits July 29, 2021 17:24

chore: add test for click today text

c9dbf23

feat: set date to today by click today text

b25325a

chore: set today text to seem clickable

2e31504

jajugoguma requested review from heenakwag, adhrinae, lja1018 and jwlee1108 July 29, 2021 08:41

dongsik-yoo reviewed Jul 29, 2021

View reviewed changes

adhrinae approved these changes Jul 30, 2021

View reviewed changes

lja1018 reviewed Aug 2, 2021

View reviewed changes

jwlee1108 approved these changes Aug 2, 2021

View reviewed changes

chore: apply code reviews

474125d

jajugoguma merged commit e7f28c9 into master Aug 12, 2021

jajugoguma deleted the feat/set-date-to-today-by-click-today-text branch August 12, 2021 03:31

feat: set date to today by click today text(fix #87) #88

feat: set date to today by click today text(fix #87) #88

Uh oh!

Conversation

jajugoguma commented Jul 29, 2021

Please check if the PR fulfills these requirements

Description

Uh oh!

dongsik-yoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adhrinae left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lja1018 left a comment

Choose a reason for hiding this comment

Uh oh!

jwlee1108 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!