-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feat/#315: 키워드 구독 기능 추가 #319
Conversation
종, 키보드, 느낌표 아이콘 추가
알림 설정, 키워드 알림 설정, 자주 묻는 질문으로 이동할 수 있는 카드 추가
자주 묻는 질문 클릭 시 faq 페이지로 라우팅하는 기능 추가
구독한 키워드 조회, 신규 키워드 구독, 키워드 삭제 요청 모킹
구독이 안된 경우, 2글자 이상이 안되는 경우, 중복된 키워드를 설정하려는 경우, 최대 키워드 개수를 넘은 경우, 서버 에러가 발생한 경우에 대한 메시지 추가
/major 주소가 누락된 문제 해결
유저가 구독한 키워드를 조회하는 기능 새로 키워드를 구독하는 기능 구독한 키워드를 취소하는 기능 추가
전체 구독 알림설정을 해야 키워드 설정이 보이도록 변경
각 키워드의 최대 글자수는 최대 15글자로 설정 키워드 설정은 최대 5개까지 설정 키워드의 글자수는 최소 2글자 중복된 키워드 설정 불가
X 버튼 클릭 시 해당 키워드를 삭제할 수 있는 기능 추가
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.
✅
src/pages/KeywordSubscribe/index.tsx
Outdated
<KeywordSubmit | ||
disabled={!checkIsAvailable()} | ||
isAvailable={checkIsAvailable()} | ||
onClick={onClickSubmit} | ||
> | ||
등록 | ||
</KeywordSubmit> |
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
폴더에 있는 공통 컴포넌트 Button
을 사용하지 않은 이유가 있나요?
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.
저도 고민이 많이 됐었는데, 일단은 common
폴더에 있는 버튼 컴포넌트를 최대한 재활용 하는 방향으로 사용하고 있긴 합니다. 테오의 프론트엔드 방에서도 질문을 했었는데 스타일이 다르다는 이유로 공통 컴포넌트를 사용하지 않고 매번 새로운 컴포넌트를 생성하기 보다 스타일을 외부에서 주입하면서 미리 구현해둔 공통 컴포넌트를 재사용하는 방향으로 간다고 하네요.. 저도 아직 어떤 방법에 더 옳은 방법인가에 대한 확신은 없지만 새로운 컴포넌트를 매번 만드는 것 보다 스타일을 주입하더라도 공통 컴포넌트를 재사용하는 방향이 더 괜찮은 것 같다는 생각은 하고 있어요~
src/pages/KeywordSubscribe/index.tsx
Outdated
<KeywordInput | ||
onChange={setInputKeyword} | ||
placeholder={KEYWORD_PAGE.PLACEHOLDER} | ||
maxLength={15} | ||
onKeyDown={handleKeyDown} | ||
value={inputKeyword} | ||
/> |
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.
여기서 onKeyDown
이벤트가 발생할 때마다 handleKeyDown
콜백 함수가 실행될 것 같은데, 이 방법보다는 form으로 감싸고 onSubmit
이벤트에 콜백 함수를 바인딩 하는건 어떨까요? 이렇게 하면 등록
버튼, 엔터 모두 정상적으로 동작합니다
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.
form 이 더 깔끔한거 같네요 수정할게요
src/pages/KeywordSubscribe/index.tsx
Outdated
fetchKeywords(); | ||
}, []); | ||
|
||
const checkIsAvailable = () => (inputKeyword.length > 1 ? true : 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.
const checkIsAvailable = () => inputKeyword.length > 1
src/pages/My/index.tsx
Outdated
<MajorCard> | ||
<Title>관리</Title> | ||
<CardIconList isSubscribe={!!subscribe}> | ||
<Icon kind="bell" color={THEME.PRIMARY} size="35" />{' '} |
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.
prettier 에서 자동으로 가독성을 위해 추가한거 같은데 지울게요~
src/pages/My/index.tsx
Outdated
<CardList> | ||
<span>알림 설정</span> | ||
<ToggleButton | ||
isOn={Boolean(subscribe)} |
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.
토글 버튼을 사용할 때는 Boolean
객체를 사용하고, 아래에서는 !!
을 사용한 이유가 있을까요?
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.
엇 동일한 결과를 출력하긴 하는데 기분에따라 사용했나보네요,, 통일하겠습니다
</CardList> | ||
</MajorCard> | ||
<MajorCard> | ||
<Title>관리</Title> |
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.
관리
섹션 내부에 있는 CardIconList
를 모두 isSubscribe
를 통해서 조건부 렌더링을 하고 있는데, 자주 묻는 질문 탭 같은 경우는 전공이 설정 되어있지 않더라도 확인을 할 수 있는 정보이니 이렇게 하는 것 보다 일단 모두 렌더링을 한 후 구독 여부로
- 토스트 메시지 출력 (전공 설정 되어있지 않음)
- 해당 페이지로 이동 시키기 (전공 설정 되어있음)
이 2가지로 분기 처리하는 로직을 사용하는건 어떨까요?
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.
여기에 대해서는 나중에 회의를 통해서 한 번 얘기해보는게 좋을거 같아요!
- 불필요한 코드 제거 {' '} - boolean 반환코드 docker-compose up로 통일
- 키워드 구독 전송을 keydown -> form 제출 형식으로 변경
피드백에 따라 최대한 공통 컴포넌트를 사용할 수 있으면 사용하도록 기존에 있는 버튼 컴포넌트 사용
피드백 수정 완료 |
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.
✅ LGTM
🤠 개요
알림 설정
,키워드 알림 설정
,자주 묻는 질문
추가💫 설명
키워드 알림
알림 설정
,키워드 알림 설정
이 보이도록 구현키워드 알림 설정
📷 스크린샷 (Optional)
Screen.Recording.2024-02-03.at.12.21.21.AM.mov