feat: 데모 피드백 사항 적용 #124
feat: 데모 피드백 사항 적용 #124
Conversation
| const handleKeyDown = (e: { key: string; shiftKey: any; preventDefault: () => void }) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault(); // 기본 Enter 키 이벤트를 막습니다. | ||
| if (content.length !== 0) submit(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Q. event parameter를 굳이 객체 분해해서 타입을 정의한 이유가 있나요?? 추가로 shiftKey는 any 외에 정의된 타입이 있을텐데 any로 정의한 이유가 있을까요?
There was a problem hiding this comment.
이벤트 자체 데이터 타입을 활용하지 못했습니다.
데이터 타입 React.KeyboardEvent로 수정하겠습니다.
| <div | ||
| className="absolute right-2 bottom-2 mr-4 border-2 border-gray-300/80 rounded-md z-10 items-center bg-violet-100/40" | ||
| hidden={content.length === 0} | ||
| > | ||
| <button | ||
| className="text-sm px-3 py-1" | ||
| onClick={submit} | ||
| type="button" | ||
| > | ||
| 작성 | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
버튼은 hidden 보다는 button의 disabled 속성으로 컨트롤 하는 게 어떨까요?
버튼이 노출이 안 되어 있으면 유저가 처음에 컨텐츠를 작성해야 작성 버튼이 나타난다고 인지하기 어려울 것 같아서요.
추가로 hidden props로 처리할 수도 있겠지만, content.length > 0 ? : null 이렇게 처리도 작성은 가능할 것 같아요!
There was a problem hiding this comment.
데모 때 피드백 반영하며,
댓글 작성시 최소 글자 제한을 없애고, 엔터키로 댓글 입력이 추가하게 되어,
작성 버튼을 사용하지 않아도 작성할 수 있게 되었습니다.
이 점에서 작성 버튼이 안 보이는 쪽이 시각적으로 깔끔하다 생각하였지만,
반면 버튼 자체에 대한 인지를 고려하지 않았던 것 같습니다.
버튼 기능은 덤으로 만든 것 같네요. 조정하겠습니다.
++ 추가로 기존의 코드에서 content.length > 0 ? : null 와 같은 형식이 많았는데,
제가 hidden 특징을 고려치않고 전반적으로 적용시켜 바꿔둔 부분이 있습니다.
이 부분의 문제를 인식해, 최신 pr에서 각 목적을 고려해 적절히 쓰이도록 조정했습니다.
| } else { | ||
| textareaRef.current ? textareaRef.current.focus() : null; | ||
| } |
There was a problem hiding this comment.
뭔가 textareRef.current && textareaRef.current.focus()를 하려고 한 것 같은데,
textareaRef.current?.focus() 로 처리하거나 / if (textareaRef.current) { ~~~ }
이렇게 작성하는 게 더 자연스럽지 않을까요?? 보통 삼항 연산자로 null까지 명시하는 건 컴포넌트 작성 부분 외에는 잘 보지 못한 것 같아요. 행위에 대해 null로 작성할 필요가 없어서..!
There was a problem hiding this comment.
textareaRef.current?.focus()로 수정하겠습니다.
처음에 옵셔널에 문제가 있어서 저렇게 했는데, 그때 코드를 잘못 만진거였네요.
컴포넌트 단에선 결과가 컴포넌트 내부로 한정되어
null 사용이 어느정도 괜찮은 것과 달리
행위에선 로직 사이에 들어니가니까 어디로 null이 튈지 몰라서 쓰면 안되는 거 였는데
허허,,생각 없이 적용만 하려 했네요..;;
| className="flex flex-row-reverse pr-4" | ||
| hidden={content.length >= 10 || content.length === 0} | ||
| > | ||
| <div className="text-gray-500"> 최소 10자 이상 입력해주세요!</div> | ||
| <div className="text-red-600/80">**</div> | ||
| </div> |
There was a problem hiding this comment.
이건 살짝 ux 적이 부분인데, 리뷰를 작성한다는 건 고객이 본인의 시간을 써 가면서 호의를 베풀어 주는 부분 중 하나인데 빨간색 폰트까지 섞여서 입력해주세요! 라고 하면 쓰면서 의도치 않은 감정을 느낄 수도 있을 것 같아요.
버튼을 눌렀을 때 10자 이상 입력해야 작성이 가능하다고 alert을 출력하거나, 10자 이상 입력하시면 댓글 등록이 가능합니다. 라고 강요가 아닌 듯하게 작성하는 게 낫지 않을까요?
There was a problem hiding this comment.
확실히 그럴 수 있네요.
alert 사용하면 가장 직관적이긴 하지만 eslint가 no-alert 기준이라,
후자 쪽으로 수정하겠습니다.
기존에 데모 때 해당 문구가 안보여서 글 작성 기능 자체가 작동을 안 한다고
인식을 줘서 이 부분을 수정하고자 했습니다.
10자 이상이면 버튼 색 변화를 주어 시각화하고,
작성 필요 시 작성란에 포커스를 주도록 보안했으니..
eslint 무시하고 alert를 쓰지 않아도 괜찮지 않을까..?합니다...
There was a problem hiding this comment.
alert 써도 되요 실제로 shop admin 쪽은 required alert 으로 이유 작성해놓고 alert 쓰고 있습니당
나중에 저희 자체 모달로 경고성 모달을 만들어서 활용하면 베스트겠지만 지금은 아직 경고성 모달에 대해 전체적으로 정의해 놓은 게 없다 보니..!
| {content.length <= 10 ? ( | ||
| <div className="border-2 border-gray-400/85 bg-gray-400 p-2">등록하기</div> | ||
| ) : ( | ||
| <div className="border-2 border-indigo-500/60 bg-blue-500 p-2">등록하기</div> | ||
| )} |
There was a problem hiding this comment.
보니까 className에 색상 관련 클래스 외에는 변하는 게 없는 것 같은데, emotion으로 styled 객체로 처리하거나 className 내부에서 조건부 처리를 하는 게 낫지 않을까요?
tailwind 쓸 때 className이 아니라 tw="" 로 작성할 수 있습니다.
추가로 button 안에 div를 추가로 넣어서 스타일링 하지 않는 방법도 있을 것 같아요!
| <div | ||
| className="absolute top-1 right-1 z-5" | ||
| hidden={userID !== props.reviewerID} | ||
| onClick={(evt) => { | ||
| evt.stopPropagation(); | ||
| }} | ||
| > |
There was a problem hiding this comment.
이 div에서 처리되는 클릭 이벤트가 따로 없는 것으로 보이는데, div에서 stopPropagation() 만 실행하도록 한 이유가 혹시 있나요??
There was a problem hiding this comment.
해당 div가 위치를 absolute로 지정해 다른 컴포넌트와 곂친 위치에 배치될 수 있습니다.
부모로 리뷰 디테일을 호출하는 div에 감싸여 있으며, absolute상 좌표로 서로 곂쳐집니다.
이것이 div 내 수정 삭제 버튼을 클릭하고자 할 때 영향을 줄 수도 있고,
때문에 이를 원천적으로 막고자 evt.stopPropagation();를 사용한 부분입니다.
| const StarArray = [1, 2, 3, 4, 5]; | ||
| const starSize = size==="big"?"50":"16"; |
There was a problem hiding this comment.
prettier 적용해주세요! 그리고 size는 기본 props를 정의해놓고 특수한 경우에만 정의하도록 하면 어떨까요?
| @@ -1,10 +1,12 @@ | |||
| interface StarProps { | |||
| size?: "big" | undefined; | |||
There was a problem hiding this comment.
?: 로 optional 선언하면 undefined가 기본으로 정의되어 있습니다. 굳이 뒤에 명시할 필요 없을 것 같아요.
|
데모 피드백 사항 반영입니다.
리뷰
리플