Conversation
There was a problem hiding this comment.
js와 jsx로 파일이름이 나뉘어진 컴포넌트들이 꽤 있네요. 혹시 어떤 이유로 파일 확장자가 다른 것인지 알 수 있을까요~?
| return ( | ||
| <ModalBack> | ||
| <ModalDiv $width={width} $height={height} $padding={padding}> | ||
| <CloseButton onClick={handleClose}> |
There was a problem hiding this comment.
닫기 버튼은 기능적으로 중요한 �요소인데요, 이 버튼에 대한 접근성을 높이기 위해 aria-label을 추가하는 것을 고려해야 합니다. 현재는 이미지 태그에만 alt 속성이 있는데, 버튼 자체에도 추가적인 설명이 있으면 좋겠습니다!
| document.body.removeChild(script); | ||
| } | ||
| }; | ||
| }, [src, error, option]); |
There was a problem hiding this comment.
useEffect 훅의 종속성 배열에 error 변수가 포함되어 있습니다. 이는 error 상태가 변경될 때마다 훅이 재실행되게 만드는데요, 의도하지 않은 스크립트 재로드를 초래할 수 있으므로, 종속성 배열에서 error를 제거하는 것이 좋습니다!
| } | ||
|
|
||
| const handleLoad = () => setLoading(false); | ||
| const handleError = () => setError(error); |
There was a problem hiding this comment.
error를 받아와서 처리하는 것이 아닌 이미 존재하는 state를 한 번 더 세팅하도록 동작할 것 같네요!
const handleError = (newError) => setError(newError) 는 어떨까요?
| const CheckImg = styled.img` | ||
| width: 14px; | ||
| height: 14px; | ||
| ${(props) => { | ||
| if (!props.$selected) | ||
| return css` | ||
| display: none; | ||
| `; | ||
| }} | ||
| `; |
There was a problem hiding this comment.
이 부분도 좋지만 더 간단히하면 컴포넌트를 렌더링 하는 곳에서 처리하는 것이 더 좋을 것 같아요! 만약 opacity였다면 props로 처리하는 것이 좋을텐데(영역을 차지하면서 보이지만 않는) 그것이 아니기 때문에 selected && <CheckImg />이런식이 더 좋을 것 같아요.
| font-weight: 600; | ||
| `; | ||
|
|
||
| export default function ChangeModal({ title, width, height, padding, setter }) { |
There was a problem hiding this comment.
여기도 마찬가지로 width, height를 optional로 받고 default 값을 세팅해주면 좋겠네요!
| cursor: pointer; | ||
| `; | ||
|
|
||
| export default function Modal({ title, width, height, padding, setter, children }) { |
There was a problem hiding this comment.
width, height의 경우는 기본 값을 미리 정해두면 어떨까요~? 다양한 곳에서 사용할 때 항상 width와 height를 넣어주는 것은 다소 번거로울 수 있을 것 같아요.
또 좋은 방법으로는 width, height를 주지 않으면 내부 컴포넌트에 fit하게 만드는 것도 방법이에요!
| @media screen and (min-width: 1124px) { | ||
| width: 1060px; | ||
| } | ||
|
|
||
| @media screen and (min-width: 769px) and (max-width: 1123px) { | ||
| width: 700px; | ||
| } | ||
|
|
||
| @media screen and (max-width: 768px) { | ||
| max-width: 687px; | ||
| } | ||
| `; |
There was a problem hiding this comment.
반응형 기준들을 상수화하는 것도 좋을 것 같습니다~!
There was a problem hiding this comment.
반응형 코드가 생각보다 많죠~ 이를 좀 더 잘 정리해서 관리하려면 어떻게 해야할지 고민해보셔도 좋을 것 같아요!
| useEffect(() => { | ||
| if (!scriptLoading && scriptError === null) { | ||
| if (Kakao?.isInitialized()) return; | ||
| Kakao?.init('3d9ed3d2882a865e3a41390794c8ec0d'); |
There was a problem hiding this comment.
카카오톡 공유 기능을 위해 외부 스크립트를 로드하고 초기화하는 로직이 잘 구현되어있네요!
하지만 이 방식은 중요한 API 키를 클라이언트 사이드 코드에 노출시킵니다. API 키를 환경 변수나 서버 사이드를 통해 관리하는 방법을 고려하는 것이 보안상 안전한데, env설정을 통해서 관리해보시는 것을 추천드려요!!!
| <StyledDiv> | ||
| <LinkInput value={url} onChange={handleChange} placeholder='링크를 추가해 보세요' /> | ||
| <StyledImg src={linkIcon} /> | ||
| <AddButton onClick={setOpenModal}>추가하기</AddButton> |
There was a problem hiding this comment.
onClick={() => setOpenModal(true)}와 같이 명시적으로 상태를 변경하는 함수를 호출하는 것이 더 좋을 것 같아요!
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게
혼자서 진행했습니다.