Conversation
Toast에서 icon을, toastHandler에서 message를 받는 대신, toastHandler에서 icon과 message를 모두 받도록 수정
🔍 Visual review for your branch is published 🔍Here are the links to: |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
BangDori
left a comment
There was a problem hiding this comment.
컨벤션에 어긋나는 부분이긴 하지만 직접 개발하였을때의 개발 비용을 무시하긴 어렵다고 생각되어, css 파일로 직접 수정하는 부분에 대해서 동의합니다.
고생하셨습니다! 리뷰를 너무 늦게 드려서 죄송하네욧..ㅠ
|
|
||
| export const Toast = () => { | ||
| return ( | ||
| <ToastContainer |
There was a problem hiding this comment.
P4: 현재 Toast 컴포넌트는 범용적으로 활용될 수 있는 shared보다는 특정 요구사항에 적합한, 추상화 수준이 매우 낮은 widgets에 적합한 것 같습니다.
ToastContainer가 변경에 유연하게 대응할 수 있도록 하기 위해, Toast 컴포넌트의 Props로 react-toastify에서 제공해주는 ToastContainer의 props를 받도록 하는 건 어떨까요? 이렇게 된다면 다양한 요구사항에 맞춰 Toast 컴포넌트를 변환할 수 있을 것이며, 높은 추상화 수준을 갖출 수 있다고 생각합니다.
There was a problem hiding this comment.
Toast props로 ToastContainerProps를 사용하자
=> 동의합니다. 이에 맞게 수정하겠습니다!
There was a problem hiding this comment.
Toast 컴포넌트를 조금 더 shared하게 사용하기 위해 props로 ToastContainerProps를 적용하고 shared에 위치하도록 하자가 제 의견이였는데, 제가 정리가 잘 안되었나 봅니다ㅠㅠ.. 다음부터 제대로 정리해서 전달드릴게요!
There was a problem hiding this comment.
P5: 메시지와 같은 부분은 다음과 같이 상수명으로 두는 것이 좋다고 생각되는데 혹시 이 부분에 대해서 어떻게 생각하시는지 수환님과 함께 얘기해보면 좋을 것 같습니다!
// 1. 객체명에 대해서만 적용
export const TOAST_MESSAGE = {
reportSuccess: '신고가 접수되었어요',
reportFail: '다시 시도해 주세요',
networkError: '인터넷 연결이 불안정해요',
commonDeleteSuccess: '댓글이 삭제되었어요',
};
// 2. 객체명과 필드명 모두 상수화
export const TOAST_MESSAGES = {
REPORT_SUCCESS: '신고가 접수되었어요',
REPORT_FAIL: '다시 시도해 주세요',
NETWORK_ERROR: '인터넷 연결이 불안정해요',
COMMON_DELETE_SUCCESS: '댓글이 삭제되었어요',
};There was a problem hiding this comment.
메시지가 고정되어 있다보니, 저도 key name을 상수형으로 주는 부분 좋은 것 같습니다!
|
|
||
| import { Input } from './Input'; | ||
|
|
||
| export default { |
There was a problem hiding this comment.
P4: 현재 Input 스토리에는 Docs가 누락되어 있어 argTypes의 내용이 정상적으로 반영되고 있지 않은 것 같아요. 작업 사항에 작성해주신 내용을 바탕으로 docs 내용을 추가해주시는 것도 나쁘지 않을 것 같습니다! (Toast Story도 동일)
| } | ||
|
|
||
| export const Input = (props: IInputProps) => { | ||
| return <input {...props} className="w-full h-full focus:outline-none" />; |
There was a problem hiding this comment.
P4: 혹시, props에서 className을 받아오는건 어떻게 생각하시나요? 지금 같은 경우에는 스타일 설정 시, div와 같은 래핑 태그로 한번 덮어서 스타일을 줘야되는 것으로 보여서요
There was a problem hiding this comment.
props로 className을 주는 방식도 고려했으나, 안그래도 className이 길어지면 관리가 어려워지는 tailwind css 특성 상
더 복잡해질 우려가 있다고 생각해 래핑 태그를 사용하는 방식을 선택했습니다!
There was a problem hiding this comment.
메시지가 고정되어 있다보니, 저도 key name을 상수형으로 주는 부분 좋은 것 같습니다!
CheckList
gap를 이용하여 flex 컨테이너 내 자식 요소간의 간격을 제어하였나요? (iOS 14 미지원으로 gap -> space-x, space-y 적용)작업 이유
작업 사항
1️⃣ Toast 구현
2️⃣ Input 구현
리뷰어가 중점적으로 확인해야 하는 부분
발견한 이슈
=> 우선은 의도대로 수정이 이뤄지는것에 중점을 맞춰 3번을 선택했는데, 팀원분들의 의견은 어떠신지 궁금합니다.
=> 여러 줄의 입력에는 TextArea가 더 적절하다고 판단되어 이를 고려해 구현하지 않았습니다.