-
Notifications
You must be signed in to change notification settings - Fork 2
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] 캘린더 메모 기능 추가 #981
The head ref may contain hidden characters: "feat/fe/\uCE98\uB9B0\uB354-\uBA54\uBAA8-\uAE30\uB2A5-\uCD94\uAC00"
[FE] 캘린더 메모 기능 추가 #981
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hafnium1923 👋🏻
안녕 루루~ 리뷰는 오랜만이다야
캘린더 메모 기능만 있을 거라고 생각했는데 범용성이 매우 높은 <Switch>
컴포넌트의 구현, storybook을 8로 마이그레이션한 과정, 그리고 다소 기술적으로 챌린징한 메모에서 <textarea>
의 글상자 크기 조절하는 로직까지 다양하게 구현했더라, 우선 정말 수고 많았어.
여러 기능들을 구현하느라 이력이랑 코드 관리하는 게 어려웠을 것 같고, 기술적으로도 여러 곳에 부딪혔을 것 같아, 그래서 그런만큼 나도 리뷰할 때 가능한 한 꼼꼼하게 보려고 노력해봤고, 수정해 볼 만한 부분들을 많이 찾은 것 같아.
각각의 작업에 대해서 전반적인 의견 나누고, 본 이야기는 코멘트로 이야기할게
storybook 7 -> 8 업데이트
캘린더 조회, 수정, 등록 모달 크기 조정
마이그레이션 하느라 수고 많았고, 모달 크기를 줄였다고 했는데 내가 봤을 때는 크기는 좀 작아졌지만 이전과 비슷하게 자연스럽다는 생각이 들었어, 그래서 크게 짚을 부분은 없었던 것 같아
캘린더 조회, 수정, 등록에서 메모관련 기능 추가
이거 msw로도 구현 잘 해줘서 테스트하기에 좋았어, 이 기능에서는 여러 기술적인 문제를 찾았는데, 해결에 도움이 될 만한 코멘트를 적어보았으니까 확인해 주면 고맙겠어, 전체적으로 싹 QA 해봤는데 확인된 문제들은 아래와 같아.
- 일정 수정 시 메모가 항상 비워지는 문제
- 메모 입력창(textarea)의 오른쪽 아래를 잡아 드래그하면 입력창 크기가 조절되면서 UI를 뚫게 되는 문제
- 공백, 줄바꿈 등의 whitespace 문자들을 입력할 수 없는 문제
종일체크박스 -> 스위치로 번경
오 이거 봤는데 장난 아니더라. 스위치 안에 글자를 넣어서 메모 버튼과 특유의 통일성이 느껴지는데 이게 굉장히 자연스럽더라. 잘 만들었어
공통 스위치 컴포넌트 구현
props 2~3개 내외의 간단한 컴포넌트를 생각했는데 굉장히 옵션이 많은만큼 상당한 범용성이 느껴지더라. 막대한 양에 혹시 리뷰하는 입장인 내가 이해하기 어려울까봐 주석이랑 storybook의 argTypes도 신경 많이 써준 게 느껴져. 한편으로는 이용 사례 자체는 일정 하나뿐인 것 같은데 이렇게 처음부터 여러 용도를 고려해 거대한 컴포넌트를 만들게 된 계기가 궁금하기도 해. 나는 너가 컴포넌트랑 싸우는 줄 알았다니까?
이 컴포넌트에 대해서는 빠르게 여러 번 스위치를 눌렀을 때의 사용성에 대한 코멘트, 그리고 prop 이름에 대한 코멘트 위주로 남겨보았어.
공통 svg 컴포넌트 구현
이 컴포넌트는 슥 훑어보긴 했는데 알쏭달쏭하더라. 혹시 더 자세히 설명해 줄 수 있어? 이 컴포넌트의 필요성, 사용 예시 같은 거가 궁금하더라고
그럼 나는 여기까지 하고 이만 자러갈게 💤 코멘트가 좀 많은 건 양해해줘, 근데 그만큼 너가 많은 작업을 해내서 코멘트도 덩달아서 많을 뿐이야.
기술적으로 챌린징한 문제들이 꽤 있었던 것 같아, 그러니까 서두르지 말고 품질을 중요시하는 방향으로 천천히 해결해보고, 하다가 막히면 언제든지 상황 공유해줘, 혹시 내가 도와줄 수 있는 게 있을지도 모르니까.
수고 많았어!
row-gap: ${({ $isMobile }) => ($isMobile ? '10px' : '16px')}; | ||
row-gap: ${({ $isMobile }) => ($isMobile ? '10px' : '10px')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모바일 여부인지와 상관없이 10px
이면 조건 분기를 없애도 될 것 같아보인다 ✏️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 좋은 포인트
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어...? 어디가 달라진거야?
딱히 바꾼 부분이 없으면 CRLF/LF 가 바뀌어서 그런가?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스토리북 8로 올라가면서 그런거 아닐까 나도 이건 건든적 없는디
render: (args) => { | ||
// eslint-disable-next-line react-hooks/rules-of-hooks | ||
const [checked, setChecked] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
왜 eslint-disable
를 썼는지 궁금해서 주석 지워봤는데, 이런 난감한 문제가 있었구나?
꼼수에 가까운 방법이기는 하지만, 이러한 방법을 써서 린터 오류를 막을 수 있어:
render: (args) => { | |
// eslint-disable-next-line react-hooks/rules-of-hooks | |
const [checked, setChecked] = useState(false); | |
render: function Function(args) { | |
const [checked, setChecked] = useState(false); |
지금 문제인 점은 useState
가 컴포넌트, 커스텀 훅 둘 중 하나에 들어있지 않아 발생하는데, 이 꼼수의 경우 함수명을 Function
과 같은 대문자로 해서 린터 에러를 피해가는 방법이야. 몇몇 개발자가 사용하는 것 같은데 별로다 싶으면 적용하지 않아도 괜찮아
아니면, .stories.tsx
에 한해 규칙을 비활성화하는 걸 고려해 볼까?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 이런게 있구만 반영했어요! 규칙 비활성화는 굳이 안해도 될 것 같아유
* 공용 Switch 컴포넌트 | ||
*/ | ||
const meta: Meta<typeof Switch> = { | ||
title: 'Common/Switch', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공용 컴포넌트를 다루고 있는 디렉토리명은 소문자 common
이고, 지금까지 공용 컴포넌트를 만들 때에도 이 부분은 폴더 대소문자 그대로 common
으로 적었으니, 이 부분도 소문자 common
으로 적어줄 수 있을까?
title: 'Common/Switch', | |
title: 'common/Switch', |
argTypes: { | ||
checked: { control: 'boolean' }, | ||
onChange: { action: 'clicked' }, | ||
size: { | ||
control: { | ||
type: 'select', | ||
options: ['xs', 'sm', 'md', 'lg'], | ||
}, | ||
description: '스위치 크기', | ||
}, | ||
variant: { | ||
control: { | ||
type: 'select', | ||
options: ['solid', 'raised'], | ||
}, | ||
description: | ||
'solid : track안에 thumb이 들어 있는 유형, raised : track보다 thumb이 큰 유형', | ||
}, | ||
readonly: { control: 'boolean' }, | ||
disabled: { control: 'boolean' }, | ||
description: { | ||
control: 'text', | ||
description: '스위치에 대한 설명이 되는 컴포넌트', | ||
}, | ||
descriptionPosition: { | ||
control: { | ||
type: 'select', | ||
options: ['top', 'bottom', 'left', 'right'], | ||
}, | ||
description: '설명 위치(상/하/좌/우)', | ||
}, | ||
onLabel: { | ||
control: 'text', | ||
description: | ||
'스위치가 켜져 있을 때의 트랙 라벨 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
offLabel: { | ||
control: 'text', | ||
description: | ||
'스위치가 꺼져 있을 때의 트랙 라벨 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
onThumb: { | ||
control: 'text', | ||
description: '스위치가 켜져 있을 때의 thumb 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
offThumb: { | ||
control: 'text', | ||
description: '스위치가 꺼져 있을 때의 thumb 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
onColor: { | ||
control: 'color', | ||
description: '스위치가 켜져 있을 때의 트랙 색상', | ||
}, | ||
offColor: { | ||
control: 'color', | ||
description: '스위치가 꺼져 있을 때의 트랙 색상', | ||
}, | ||
thumbOnColor: { | ||
control: 'color', | ||
description: '스위치가 켜져 있을 때의 thumb 색상', | ||
}, | ||
thumbOffColor: { | ||
control: 'color', | ||
description: '스위치가 꺼져 있을 때의 thumb 색상', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argTypes
랑 control
을 많이 신경썼구나 👍🏻
몇 개 건드려보니까
- 라디오 형태로 고를 수 있는 형태의 값들을 셀렉트 형태로 고를 수 있도록 바꾸었고
- 실제로는 문자열을 적을 수 있는
description
과 같은 prop에서Set Object
와 같이 잘못된 입력 UI로 추론되어 나오는 거를Set String
과 같이 올바른 UI로 보이도록 개선해주었구나
섬세한데?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감삼당감삼당
setIsDescriptionMaxLength(false); | ||
} | ||
|
||
handleDescriptionChange(textarea.value.trim().slice(0, 100)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100글자를 넘어서 칠 수 없도록 차단하는 로직이구나? 취지는 이해가 가는데, trim()
을 사용한 경우라면 양끝의 whitespace를 제거하게 되고, 이 로직은 사용자가 메모를 한 글자 한 글자 칠 때마다 실행되는지라, 사용자가 공백이나 줄바꿈 등의 문자를 입력할 수 없게 되는 원인이 될 것 같아
"수학 시러"
를 메모로 입력하려고 한다면, "수학 "
까지 입력한 시점에서 "수학 " → "수학"
이 되어버려서 진행이 안 될 것 같아, 이 부분을 한 번 수정해보면 어떨까?
꼭 trim()
을 해야 한다면, 사용자가 입력할 때는 slice(0, 100)
만 하고 폼에서 최종 전송을 할 때 trim()
을 하는 것도 하나의 방법이 될 수 있을 것 같아
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오홍 그럼 그냥 trim안쓰지몽~,~
white-space: normal; | ||
overflow-wrap: break-word; | ||
display: inline-block; | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옼 이런게..!! 적용해둘게용
@@ -52,6 +81,7 @@ export const useScheduleAddModal = (clickedDate: Date) => { | |||
title, | |||
startDateTime, | |||
endDateTime, | |||
description, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useScheduleEditModal.ts
의 mutate 로직에도 description
을 추가해 줘야 할 것 같아, description
이 빠졌어
그래서 일정 생성 시 메모를 적은 경우에는 올바르게 적용되지만, 일정 수정 시에는 메모가 항상 undefined
가 되서 메모가 사라지는 문제가 있어
빠진 description
을 넣으면 의도한 대로 잘 작동할거야
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋㅋㅋㅋ아니 사실은 이거 내가 혼자만들고 있는 디자인시스템에서 긁어온거거든..? 공용 컴포넌트 하나씩 만들고 있었어서.. 근데 디자인시스템이니까 사용방법을 써야할 거 아냐 그래서 완전 열심히 써둠ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 긁어오면서 설명 지울필요를 못느껴서 그대로 뒀서🤪
args: { | ||
size: 'md', | ||
checked: false, | ||
onChange: () => console.log('Switch Changed'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코멘트는 반영하라는 의미가 아니고, 그냥 짤막하게 기능 하나 공유하러 왔어
storybook 8에서는 @storybook/test
에서 제공하는 fn()
이라는 spy 함수를 사용할 수 있는데, 단순히 로그를 찍는 게 목적이라면 간편하게 사용할 수 있는 녀석이야
import는 이렇게 하고
import { fn } from '@storybook/test';
사용할 때는 그냥 fn()
만 붙여주면 돼!
onChange: fn(),
그렇게 하면 해당 이벤트가 트리거 될 때마다 Action 탭에 로그가 떠
괜찮아 보인다면, 나중에 fn()
을 사용하는 방안으로 마이그레이션 해보는 것도 나쁘지 않을 것 같아서 공유해봤어, 나는 이미 다른 프로젝트에서 쓰고 있는데, alert
을 써서 매번 화면 멈춰서 짜증나는 문제들이 말끔히 해결됐어. console.log
는 그나마 덜 방해되는데, 아무래도 다른 로그에 묻힐 수가 있겠지
참고: https://storybook.js.org/docs/essentials/actions
이건 다른 프로젝트에서 사용한 예시
_2025_01_08_01_15_41_557.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
올 좋은데? 사용해볼게
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 바로 적용해줬구나 ㅋㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안뇽하세용 토쨩~~~🤭
열심히 반영해서 돌아온 룰루랄라입니다요!
꼼꼼한 QA와 리뷰 고마워 혼자한다고 했는데 역시 100%는 어렵네ㅎ..
대부분 코멘트로 달아두었으니 확인 부탁드려요!
또 스위치 관련해서는 너가 리뷰해준대로 스로틀링을 주석처리해놨어! 지금 테스트해보면 바로바로 반응하는 스위치를 볼 수 있을거야. 주석처리한 이유는 코멘트에 달아두었듯 아예 없앨지, 아니면 조절 가능하게 할지 의견을 물어본 상태라 그래! 한번 더 리뷰해주면 그에 맞게 반영해볼게
props 2~3개 내외의 간단한 컴포넌트를 생각했는데 굉장히 옵션이 많은만큼 상당한 범용성이 느껴지더라. 막대한 양에 혹시 리뷰하는 입장인 내가 이해하기 어려울까봐 주석이랑 storybook의 argTypes도 신경 많이 써준 게 느껴져. 한편으로는 이용 사례 자체는 일정 하나뿐인 것 같은데 이렇게 처음부터 여러 용도를 고려해 거대한 컴포넌트를 만들게 된 계기가 궁금하기도 해. 나는 너가 컴포넌트랑 싸우는 줄 알았다니까?
ㅋㅋㅋㅋㅋㅋㅋ코멘트에도 달아뒀지만 저의 다른 토이플젝에서 긁어온거라 그렇답니다ㅎㅅㅎ 이왕 만들어진거 굳이 지우기 싫어서 그냥 해놨어요ㅎ.ㅋ 이해하기 쉬웠다니 제 개노가다가 의미있어보여서 다행입니다.
SVG컴포넌트는 svg 사이즈 조절과 svg의 이름 자동완성을 중점으로 만들었는데 댓글 한번 읽어주면 감사하겠어욧✨
이번에도 리뷰 잘부탁해용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스토리북 8로 올라가면서 그런거 아닐까 나도 이건 건든적 없는디
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우리 지금 svg 파일 하나하나 넣어두고 import해서 사용하잖아 근데 얘들은 사이즈가 정해져있단 말이지? 그래서 만약에 14 * 14인 svg를 48 * 48로 만들고 싶다면 선택자로 지정하던가, 아니면 이것저것 밖에서 지정해서 넣어야했어.
svg컴포넌트는 이런 문제점을 해결하기 위해서 만들었던 컴포넌트야. 일단 사용할 이름만을 type으로 받는데 자동완성도 가능해서 svg의 이름을 하나하나 기억할 필요도 없고, size와 기타 등등의 prop을 이용해서 svg를 컨트롤 할 수 있어.
다만 svg 컴포넌트를 이용해서 size조절하기 위해서 가장 큰 조건이 있는데 바로 svg가 아래처럼 width height모두 100%로 되어있어야해
<svg width="100%" height="100%" .../>
이 컴포넌트는 다른 프로젝트에서 만들어두었던 컴포넌트인데 팀바팀에서도 있으면 좋을 것 같아서 가져왔어용
render: (args) => { | ||
// eslint-disable-next-line react-hooks/rules-of-hooks | ||
const [checked, setChecked] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 이런게 있구만 반영했어요! 규칙 비활성화는 굳이 안해도 될 것 같아유
argTypes: { | ||
checked: { control: 'boolean' }, | ||
onChange: { action: 'clicked' }, | ||
size: { | ||
control: { | ||
type: 'select', | ||
options: ['xs', 'sm', 'md', 'lg'], | ||
}, | ||
description: '스위치 크기', | ||
}, | ||
variant: { | ||
control: { | ||
type: 'select', | ||
options: ['solid', 'raised'], | ||
}, | ||
description: | ||
'solid : track안에 thumb이 들어 있는 유형, raised : track보다 thumb이 큰 유형', | ||
}, | ||
readonly: { control: 'boolean' }, | ||
disabled: { control: 'boolean' }, | ||
description: { | ||
control: 'text', | ||
description: '스위치에 대한 설명이 되는 컴포넌트', | ||
}, | ||
descriptionPosition: { | ||
control: { | ||
type: 'select', | ||
options: ['top', 'bottom', 'left', 'right'], | ||
}, | ||
description: '설명 위치(상/하/좌/우)', | ||
}, | ||
onLabel: { | ||
control: 'text', | ||
description: | ||
'스위치가 켜져 있을 때의 트랙 라벨 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
offLabel: { | ||
control: 'text', | ||
description: | ||
'스위치가 꺼져 있을 때의 트랙 라벨 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
onThumb: { | ||
control: 'text', | ||
description: '스위치가 켜져 있을 때의 thumb 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
offThumb: { | ||
control: 'text', | ||
description: '스위치가 꺼져 있을 때의 thumb 내 들어갈 텍스트 또는 아이콘', | ||
}, | ||
onColor: { | ||
control: 'color', | ||
description: '스위치가 켜져 있을 때의 트랙 색상', | ||
}, | ||
offColor: { | ||
control: 'color', | ||
description: '스위치가 꺼져 있을 때의 트랙 색상', | ||
}, | ||
thumbOnColor: { | ||
control: 'color', | ||
description: '스위치가 켜져 있을 때의 thumb 색상', | ||
}, | ||
thumbOffColor: { | ||
control: 'color', | ||
description: '스위치가 꺼져 있을 때의 thumb 색상', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감삼당감삼당
args: { | ||
size: 'md', | ||
checked: false, | ||
onChange: () => console.log('Switch Changed'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
올 좋은데? 사용해볼게
const throttledOnChange = useCallback(() => { | ||
if (throttleTimeout.current === null) { | ||
onChange(); | ||
throttleTimeout.current = window.setTimeout(() => { | ||
throttleTimeout.current = null; | ||
}, 500); | ||
} | ||
}, [onChange]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 스로틀링 적용한거만 지우면 첨부한 영상과 동일하게 작동할 수 있어! 스위치가 보통 on/off처럼 어떤걸 조작하는거니 너무 자주 변경하면 연결된 함수가 너무 자주 일어나지 않을까? 하는 생각으로 적용해둔거야. 필요없을 것같으면 스로틀링을 아예 지워버릴까? 아니면 이것또한 props로 지연이 필요한지를 받는게 나을 것 같아? 의견 알려줘!
row-gap: ${({ $isMobile }) => ($isMobile ? '10px' : '16px')}; | ||
row-gap: ${({ $isMobile }) => ($isMobile ? '10px' : '10px')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 좋은 포인트
white-space: normal; | ||
overflow-wrap: break-word; | ||
display: inline-block; | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옼 이런게..!! 적용해둘게용
setIsDescriptionMaxLength(false); | ||
} | ||
|
||
handleDescriptionChange(textarea.value.trim().slice(0, 100)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오홍 그럼 그냥 trim안쓰지몽~,~
http.get('/api/auth/oauth/google/login', () => { | ||
return HttpResponse.json({ | ||
googleLoginUrl: '/login?accessToken=aaaa&refreshToken=bbbb', | ||
}); | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
크크 이렇게 하면 되지 않을까 싶었어!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const throttledOnChange = useCallback(() => { | ||
if (throttleTimeout.current === null) { | ||
onChange(); | ||
throttleTimeout.current = window.setTimeout(() => { | ||
throttleTimeout.current = null; | ||
}, 500); | ||
} | ||
}, [onChange]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스위치가 보통 on/off처럼 어떤걸 조작하는거니 너무 자주 변경하면 연결된 함수가 너무 자주 일어나지 않을까? 하는 생각으로 적용해둔거야.
루루가 말한대로, 연결된 함수의 경우 사용처에 따라 자주 일어나도 될만한 로직이 될 수도 있을 것 같고, 자주 일어날 경우 퍼포먼스 면에서 위험한 로직이 될 수도 있다는 것에는 나도 동의해. 다만 이 컴포넌트는 공용 컴포넌트이니 스로틀링을 강제한다면 스로틀링이 필요 없는 케이스에서는 오히려 방해가 될 수 있을 거라는 생각이 들었어
- 지금은 사라진 종일 일정 체크박스의 경우에도 체크박스를 설정/해제할 때 스로틀링 없이 바로 UI가 변화하도록 구현했었는데, 체크박스가 바뀌었을 때 즉시 UI가 반영되지 않고 약간의 시간 후에 반영된다면 그거대로 어색할 것 같다는 생각이 드는 것 같아. 이런 케이스들은 무거운 연산을 하는 것도 아니기에 스로틀링 없이 즉시 반영을 하는 것이 자연스러울 것 같다는 생각이 들었어
필요없을 것같으면 스로틀링을 아예 지워버릴까? 아니면 이것또한 props로 지연이 필요한지를 받는게 나을 것 같아? 의견 알려줘!
- 지연이 필요한 케이스(스위치를 클릭할 경우 네트워크 비동기 요청이 일어나는 경우, 무거운 연산을 수행하는 경우 등)가 여러 개 예상되거나 이후 이러한 케이스가 추가될 지 불확실하다면, 아니면 그냥 고도화된 컴포넌트를 만들어보고 싶다면 -- 지연이 필요한지를 props로 받으면 좋을 것 같고
- 단 한 곳에서만 스로틀링 로직을 사용하는 것이 예상된다면 props로 이 옵션을 두지 않고 그 스로틀링 로직을 부모 컴포넌트로 옮기면 좋을 것 같다는 생각이 들어
추가로, 퍼포먼스를 생각해 스위치에 스로틀링 로직을 넣는다면 -- 스위치의 켜짐/꺼짐 UI 자체에는 스로틀링을 넣지 않고, 클릭할 때마다 즉시 반영되도록 하되, 연결된 함수를 실행하는 로직만을 스로틀링하는 방법도 있을 것 같아. 사용자 입장에서는 스위치 자체는 빠르게 클릭했을 때 클릭에 전부 반응하는 것처럼 보이지만, 실제로는 필요한 적은 횟수만 연결된 함수가 실행되는 거지, 이러한 로직은 판단하에 루루가 적합하다고 생각하는 방법을 써 주면 될 것 같아
checked: boolean; | ||
onChange: () => void; | ||
size?: SwitchSize; | ||
variant?: 'solid' | 'raised'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 Chakra에서 이런 단어를 사용하는구나
처음 알았어, 다음부터는 Chakra도 컴포넌트 레퍼런스로써 생각할게
이 이름이 싫다고 이야기했던 게 아니라 "Carousel" 이라는 단어처럼 처음 보는 단어를 봤을 때 잘 이해가 안 되었어서 그랬던 거야~ 그러니까 그대로 둬도 좋을 것 같아
여담으로 조금 더 생각해봤는데 variant가 solid
인 컴포넌트는 손잡이와 트랙의 높이가 같아 평평해 보이는데, raised
인 컴포넌트인 손잡이가 트랙 위를 덮고 있는 형태고 그림자도 씌워져 있어 전반적으로 손잡이가 "위로 올라와 있는" 듯한 느낌이 드네? 이게 이유일수도?
@@ -18,6 +18,8 @@ export const TIME_TABLE = arrayOf(48).map((_, i) => { | |||
|
|||
export const SCHEDULE_CIRCLE_MAX_COUNT = 3; | |||
|
|||
export const SCHEDULE_DESCRIPTION_MAX_LENGTH = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름 한 번 야무지구만
args: { | ||
size: 'md', | ||
checked: false, | ||
onChange: () => console.log('Switch Changed'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 바로 적용해줬구나 ㅋㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이왕 상수를 적용해 줬다면 이 컴포넌트에도 상수를 적용해 줄 수 있을 것 같아보인다
메모를 작성해주세요.(최대 ${상수}자)
최대 ${상수}자까지 입력가능합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그거 고민하긴 했는데 문자열 너무 더러워보여서 뺐었음ㅋㅋ 해둘게
<div> | ||
{isDescription && ( | ||
<S.DescriptionTextarea | ||
rows={1} | ||
placeholder="메모를 작성해주세요.(최대 100자)" | ||
value={schedule.description} | ||
onChange={handleDescriptionInput} | ||
required | ||
/> | ||
)} | ||
<S.DescriptionDiv $isDescription={isDescription}> | ||
<S.DescriptionTextarea | ||
rows={1} | ||
placeholder="메모를 작성해주세요.(최대 100자)" | ||
value={schedule.description} | ||
onChange={handleDescriptionInput} | ||
required | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
textarea를 마운트/언마운트 하는 방법에서 display: none
을 토글하는 방법으로 바꿔서 DOM 트리에는 나타나도록 유지하는 방법으로 바꾼 것 같은데 기능상으로 문제가 생길 수 있을 것 같아
display: none
이 적용되어 화면에 나타나지는 않는 요소일지라도 폼 전송에는 여전히 관여하고, 값도 지니기 때문에 "메모" 버튼을 누르지 않아 메모 란이 보이지 않는 상황에서 사용자가 "등록" 버튼을 누를 경우 메모 값을 지니는 textarea가 폼의 전송을 막는 문제가 있을 거야. textarea는 값을 무조건 입력해야만 하는 required
attribute를 지니고 있고 공란이니까...
display: none
을 토글하는 방법을 더 선호한다면 disabled
attribute도 같이 조작해 보는 건 어때? disabled
attribute가 붙은 요소는 validation에서 제외되니까 활용이 가능할거야
TMI: 에러 발생 원인
An invalid form control with name='' is not focusable.
"등록" 버튼을 누를 경우 발생하는 에러인데
- textarea의 값이 비어 있어 form validation에 실패해. 그러면 기본 동작인 "해당 인풋을 포커싱하고 [이 입력란을 작성하세요.] 기본 메시지를 띄워주기" 를 수행해야 돼.
- 그런데 textarea는
display: none
이기 때문에 포커싱을 할 수가 없어. 이 이유로 에러가 발생해
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
홀뤼,, 이거 안보이게 해둔게 display: none을 안하고 isDescription 가 있을때만 컴포넌트가 등장하도록 했었거든? 그니까 왜인진 모르겠는데 돔상에 보이면 안되는 메모 입력 영역이 div로 잡히는거야. 그래서 gap이 2배로 적용돼서 UI 깨져서 일부러 None으로 해둔거였음..ㅠ disabled로 조작해볼게 ㅎ required 땜에 그랬군앙.ㅋ.. 암튼 해결해둘게요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여러가지로 반영하느라 수고 많았어! typo가 하나 난 것 같아서 코멘트 하나만 적어봤고, 그거 고치면 Merge 하면 될 것 같아
그 이외에도 이번 PR에서 마지막으로 반영하고 싶은 거 있으면 적절히 반영하면 될 것 같아
✅
<Text size="xs"> | ||
({schedule.description.length} / $ | ||
{SCHEDULE_DESCRIPTION_MAX_LENGTH}자) | ||
</Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test Results160 files 160 suites 31s ⏱️ Results for commit 7f94c2d. |
7f94c2d
to
584a7a8
Compare
…-teams/2023-team-by-team into feat/fe/캘린더-메모-기능-추가
[FE] 캘린더 메모 기능 추가
이슈번호
PR 내용
storybook 7 -> 8 업데이트
캘린더 조회, 수정, 등록 모달 크기 조정
캘린더 조회, 수정, 등록에서 메모관련 기능 추가
종일체크박스 -> 스위치로 번경
공통 스위치 컴포넌트 구현
공통 svg 컴포넌트 구현
의논할 거리
msw에서 왜 한달기간조회할 때 해당하는 스케줄이 안뜰까.? 23년도까지는 잘돌아가는 것 같은디.. 흠..