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

[FE] 시간 선택 Dropdown value 수정 #340

Merged
merged 7 commits into from
Sep 5, 2024
Merged

Conversation

Largopie
Copy link
Contributor

@Largopie Largopie commented Sep 4, 2024

관련 이슈

작업 내용

2024-09-04.11.27.37.mov

특이 사항

useTimeRangeDropdown util 함수 수정

export function addHoursToCurrentTime(currentTime: string, hours: number) {
  const [currentHours, currentMinutes] = currentTime.split(':').map(Number);

  return (
    String(currentHours + hours).padStart(2, '0') + ':' + String(currentMinutes).padStart(2, '0')
  );
}

해당 함수가 시간, 분을 반환할 때 숫자로 반환하기 때문에 한자리 숫자일 때 앞에 ‘0’을 붙이도록 수정했습니다.

이에 대한 테스트 케이스도 추가했어용

리뷰 요구사항 (선택)

@Largopie Largopie added 🐈 프론트엔드 프론트엔드 관련 이슈에요 :) 🛠️ 픽스 버그를 해결했어요 :) labels Sep 4, 2024
@Largopie Largopie added this to the 5차 데모데이 milestone Sep 4, 2024
@Largopie Largopie self-assigned this Sep 4, 2024
Copy link

github-actions bot commented Sep 4, 2024

Test Results

5 tests   5 ✅  3s ⏱️
1 suites  0 💤
1 files    0 ❌

Results for commit 2fa977d.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@Yoonkyoungme Yoonkyoungme left a comment

Choose a reason for hiding this comment

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

낙타 고생했습니다~!
가벼운 의견을 남겨두었는데, 확인 부탁드려요!

Comment on lines 59 to 69
it('시작 시간(startTime)을 선택하면, 끝 시간은 1시간 이후로 자동 설정된다.', () => {
const CHANGE_START_TIME = '18:00';
const CHANGE_END_TIME = '19:00';
const { result } = renderHook(() => useTimeRangeDropdown());

act(() => {
result.current.handleStartTimeChange(CHANGE_START_TIME);
});

expect(result.current.endTime.value).toBe(CHANGE_END_TIME);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

테스트 굿👍👍
궁금해서 찾아봤는데, 구글 캘린더에서도 시작 시간을 선택하면 자동으로 끝 시간을 1시간(기본값, 조절 가능) 뒤로 설정해주고 있네요!

2024-09-04.4.49.48.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

뭐야 낙타 구글가는거야?

Copy link
Contributor

@Yoonkyoungme Yoonkyoungme Sep 4, 2024

Choose a reason for hiding this comment

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

[P3]

51번 라인 주석에 오타있습니다! (반화 → 반환)

// 현재 시간에서 1시간 더한 시간을 반화해주는 함수(@낙타)

Comment on lines 59 to 52
it.each([
['18:00', '19:00'],
['06:00', '07:00'],
])(
'시작 시간(startTime)을 선택하면, 끝 시간은 1시간 이후로 자동 설정된다.',
(startTime, endTime) => {
Copy link
Contributor

@Yoonkyoungme Yoonkyoungme Sep 4, 2024

Choose a reason for hiding this comment

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

[P3]

템플릿 리터럴을 사용해서 각 테스트 케이스의 입력값과 예상 결과를 설명에 포함하는건 어떨까요?

it.each([
  ['18:00', '19:00'],
  ['06:00', '07:00'],
])('시작 시간(%s)을 선택하면 끝 시간(%s)은 1시간 뒤로 자동 설정된다', (startTime, endTime) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오호 이거 좋네요!

Copy link
Contributor

@hwinkr hwinkr left a comment

Choose a reason for hiding this comment

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

🐞 버그 잡느라 고생했어요! 낙타~ 🐪🐪🐪

@@ -52,5 +52,5 @@ export function isTimeSelectable(startTime: string, endTime: string) {
export function addHoursToCurrentTime(currentTime: string, hours: number) {
const [currentHours, currentMinutes] = currentTime.split(':').map(Number);

return currentHours + hours + ':' + currentMinutes;
return currentHours + hours + ':' + String(currentMinutes).padStart(2, '0');
Copy link
Contributor

Choose a reason for hiding this comment

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

[C]

+ ':'로 인해서, addHoursToCurrentTime 해당 함수의 반환값이 문자열이 될 것 같은데 + 연산자를 활용하는 것 보다 템플릿 리터럴과 $로 묶어서 반환하는 것은 어떨까요? 개인적인 취향이기도 하고 더 깔끔하다고 생각해서 코멘트로 달아봅니다~

@Largopie
Copy link
Contributor Author

Largopie commented Sep 5, 2024

  it('시작 시간(startTime)을 선택하면 끝 시간(endTime)은 시작 시간(startTime)의 1시간 이후로 설정된다.', () => {
    const CHANGE_TIME = '01:00';
    const CHANGE_TIME_AFTER_HOUR = '02:00';
    const { result } = renderHook(() => useTimeRangeDropdown());

    act(() => {
      result.current.handleStartTimeChange(CHANGE_TIME);
    });

    expect(result.current.endTime.value).not.toBe(CHANGE_TIME_AFTER_HOUR);
  });

잘못된 테스트 케이스 삭제조치 추가로 했고, 남겨주신 리뷰도 반영햇습니다요~

@Largopie Largopie merged commit 4e02d54 into develop Sep 5, 2024
5 checks passed
@Largopie Largopie deleted the fix/337-dropdown branch September 5, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐈 프론트엔드 프론트엔드 관련 이슈에요 :) 🛠️ 픽스 버그를 해결했어요 :)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FE] 시간 선택 Dropdown value 수정 :)
3 participants