-
Notifications
You must be signed in to change notification settings - Fork 0
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
[3주차 기본/심화/공유 과제] 1 to 50 게임 #4
base: main
Are you sure you want to change the base?
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.
과제 수고하셨습니다!! 전체적으로 컴포넌트랑 utils의 분리가 너무 잘 되어 있어서 코드를 읽는 게 너무 편했던 것 같습니다! 그리고 children의 활용이나 폴더 구조 설계도 뭐가 더 좋은 설계인지 고민하시고 작성을 하시는 게 보이는 것 같아서 보면서도 배울 점이 많았던 것 같습니다 👍
{rankings.map((record, index) => ( | ||
<TableRow key={index}> | ||
<TableData>{record.currentTime}</TableData> | ||
<TableData>{record.level}</TableData> | ||
<TableData>{record.playTime}</TableData> | ||
</TableRow> | ||
))} |
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.
p3 : 개인 스타일마다 다르겠지만 map()
에서도 구조 분해 할당이 가능합니다! 그래서 record
를 반복적으로 작성하지 않고,
{rankings.map(({currentTime, level, playTime}, index) => (
<TableRow key={index}>
<TableData>{currentTime}</TableData>
<TableData>{level}</TableData>
<TableData>{playTime}</TableData>
</TableRow>
))}
이렇게도 사용이 가능합니다. 하지만 map
을 돌리는 요소가 뭔지 확인하기 위해 record
를 명시적으로 작성하는 것도 좋은 방법인 것 같습니다!!
const [rankings, setRankings] = useState(() => { | ||
const storedData = JSON.parse(localStorage.getItem("gameData")) || []; | ||
return sortRanking(storedData); | ||
}); |
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.
p3 : useState
의 초기 값에 로컬 스토리지에서 storedData
를 가져오는 로직을 넣은 것이 useEffect
의 사용을 대신한 것인가요? 저는 useEffect
를 사용을 지양해야 한다는 것을 생각을 안하고 이 ranking
데이터를 가져올 때 사용한 것 같은데, useState
에서
이렇게 초기 값을 설정해주면 처음 컴포넌트 마운팅 되고 이 데이터를 가져오는 것인지 궁금합니다!
const SelecteLevel = ({ level, onLevelChange }) => { | ||
return ( | ||
<SelecteLevelContainer> |
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.
(select 오타 났습니다!)
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.
(select 오타 났습니다!)
헙! 감사합니다
select { | ||
padding: 0.5rem; | ||
font-size: 3rem; | ||
font-weight: 500; | ||
border-radius: 10px; | ||
border: 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.
p2 : border-radius
에서 단위를 사용하실 때 rem
을 사용하실 때도 있고 px
을 사용하실 때도 있는데 이에 대한 기준이 있을까요? 저도 최근 rem
을 적극적으로 사용해보려고 노력 중인데 border-radius
와 같이 화면 크기에 상관이 없는 속성에는 px
을 사용하고 그 외에 rem
을 사용하고 있습니다. 근데 이에 대해서 주변 분들의 관점이 무조건 하나로 통일해라(ex. rem) 혹은 제가 사용한 것처럼 px/rem
을 조금씩 섞어서 써도 괜찮다는 의견으로 나뉘어서 어떤 생각을 가지신지 궁금했습니다!!
(조언해주실 내용이 있다면 해주세요!!)
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.
p2 :
border-radius
에서 단위를 사용하실 때rem
을 사용하실 때도 있고px
을 사용하실 때도 있는데 이에 대한 기준이 있을까요? 저도 최근rem
을 적극적으로 사용해보려고 노력 중인데border-radius
와 같이 화면 크기에 상관이 없는 속성에는px
을 사용하고 그 외에rem
을 사용하고 있습니다. 근데 이에 대해서 주변 분들의 관점이 무조건 하나로 통일해라(ex. rem) 혹은 제가 사용한 것처럼px/rem
을 조금씩 섞어서 써도 괜찮다는 의견으로 나뉘어서 어떤 생각을 가지신지 궁금했습니다!!(조언해주실 내용이 있다면 해주세요!!)
px은 절대 단위이기 때문에 사용자가 설정한 브라우저 기본 폰트 크기나 화면 크기와 상관없이 고정된 크기를 유지하게 됩니다. 그렇기 때문에 저같은 경우는 진혁님과 동일하게 border-radius
와 border
값에만 px
을 사용합니다. 왜냐하면 화면 크기와 상관없이 border-radius
를 일정하게 유지 할 수 있고, border의 두께는 화면 크기와 상관없이 일정하게 유지되어야 하는 경우가 많기 때문인데요. 무조건 하나로 통일해라도 의견 중에 하나일 수 있겠지만, 자신만의 주관을 가지고 통일성을 유지(특정한 상황에만 사용 ex: border-radius
와 border
값에만 사용)한다면 어떠한 방법도 괜찮다고 생각합니다. 실제로 제가 진행한 프로젝트에서도 border-radius
와 border
값에만 px
을 사용하였답니다.
export const getGameBoardProps = (level) => { | ||
switch (level) { | ||
case "level1": | ||
return { initialLength: 9, remainingLength: 9, gridLength: 3 }; | ||
case "level2": | ||
return { initialLength: 16, remainingLength: 16, gridLength: 4 }; | ||
case "level3": | ||
return { initialLength: 25, remainingLength: 25, gridLength: 5 }; | ||
default: | ||
return { initialLength: 9, remainingLength: 9, gridLength: 3 }; | ||
} | ||
}; |
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.
p2: switch case
문으로 쓴 현재 코드도 좋지만, level
이 많아지는 확장성(혹은 적어지는 경우도)을 고려한다면 level
을 객체로 상수화해서 사용하는 것도 좋을 것 같습니다. 그리고 default
가 case "level1"
와 중복이 되는 경우도 해결할 수 있을 것 같습니다!
코드 예시)
const gameLevels = {
level1: { initialLength: 9, remainingLength: 9, gridLength: 3 },
level2: { initialLength: 16, remainingLength: 16, gridLength: 4 },
level3: { initialLength: 25, remainingLength: 25, gridLength: 5 },
};
export const getGameBoardProps = (level) => {
return gameLevels[level] || gameLevels.level1;
};
const reset = () => { | ||
resetGame(initialLength, remainingLength, { | ||
setInitialnums, | ||
setUpcomingNums, | ||
setCurrentNumber, | ||
setGameComplete, | ||
setClickedIndexes, | ||
onReset, | ||
}); | ||
}; |
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.
p3: resetGame utils
을 분리를 잘 하셨는데 이를 직접 사용하는 게 아니라 reset
에 감싸서 사용하는 이유는 게임 관련 상태를 GameBoard
컴포넌트에 위치 시키기 위함일까요? 혹시 그렇다면 utils
대신 custom hook
으로 Game 관련된 hook
을 분리해서 state
를 같이 관리하고 그 hook
을 그대로 import해서 사용하는 것은 어떨까요?
이번에 제 과제를 할 때 시간 투자를 많이 못해서 함수 분리(utils나 custom hook 등)를 거의 못해서.. 이런 리뷰를 드리기 조금 죄송스럽지만 이에 대해서 어떤 의견을 가지고 계신지 궁금합니다!
) : ( | ||
<EmptySpace key={index} /> | ||
) |
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.
p1: 여기서는 key
가 필요 없을 것 같습니다!
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.
p1: 여기서는
key
가 필요 없을 것 같습니다!
맞네요! 감사합니다
{gameComplete && ( | ||
<CompleteModal onClose={handleCloseModal}> | ||
<Message>게임 기록: {currentTime}</Message> | ||
<CloseButton onClick={handleCloseModal}>닫기</CloseButton> | ||
</CompleteModal> | ||
)} | ||
</GameBoardContainer> |
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.
p3: 여기서
<CloseButton onClick={handleCloseModal}>닫기</CloseButton>
를 따로 두신 것 같아서
onClose={handleCloseModal}
가 필요 없을 것 같은데 혹시 다른 역할이 있을지 궁금합니다!
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.
p3: 여기서
<CloseButton onClick={handleCloseModal}>닫기</CloseButton>
를 따로 두신 것 같아서onClose={handleCloseModal}
가 필요 없을 것 같은데 혹시 다른 역할이 있을지 궁금합니다!
처음에 내부 모달 내부 요소들을 chidren
으로 넘겨서 사용하기전에 작성하였던 부분인데 지우지 못했네요. 꼼꼼한 코드리뷰 감사합니다!
const NumberButton = styled.button.withConfig({ | ||
shouldForwardProp: (prop) => prop !== "isClicked", | ||
})` |
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.
p3: 이렇게 작성해도 styled-components 스타일링을 위해 props는 전달되지만 실제 DOM 요소에는 전달되지 않는군요!!!
단순히 저는 $
키워드를 써서 이 작업을 해줬는데, shouldForwardProp
는 어떤 prop
을 DOM에 전달할지 또는 전달하지 않을지 명시적으로 제어할 수 있다는 점이 다른 것 같네요! $
로 할 수 없는 조건을 추가하거나 복잡한 로직을 사용할 때 더 좋을 것 같아요. 좋은 지식 배워갑니다👍
const CompleteModal = ({ children }) => { | ||
return ReactDOM.createPortal( | ||
<Overlay> | ||
<Modal>{children}</Modal> | ||
</Overlay>, | ||
document.getElementById("modal-root") | ||
); | ||
}; |
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.
p3: children
활용과 컴포넌트 분리가 너무 깔끔해서.. 보기 너무 편하네요!!
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.
코드도 너무 깔끔해서 알아보기도 쉽고 모듈화도 정말 잘 되어있는... 배울 점이 많은 코드였던 것 같습니다! 저도 고봉밥 코드리뷰를 달고 싶었는데,,, ㅎㅎ 코드를 보면서 개인적으로 의문점이 드는 부분들을 직접 찾아보다보니 오히려 너무 좋은 공부가 되었던 것 같아요 그러면서 깨달은 건 확장성 고려에 대해서도 신경을 쓴 게 느껴졌어요!
그리고 진~짜 오랫동안 건휘님 코드를 보고 그랬는데 코리를 썼다 지웠다 x100을 하다보니 남은 게 4개밖에 없네요 ,,,,,,, (절대 대충 보고 그런 거 아님,, 진짜 진심 오래 꼼꼼히 봤어요) 진혁님도 코드리뷰 너무 꼼꼼히 달아주셔서 더해서 배울 수 있었습니다 ㅠ,,ㅠ 역시 나리스 스장은 다르다!!!! 최고 😉 😉 😉 😉
const NumberButton = styled.button.withConfig({ | ||
shouldForwardProp: (prop) => prop !== "isClicked", | ||
})` |
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.
p3:
오,,, shouldForwardProp
을 걸 이 코드를 통해 처음 알게 됐어요!
isClicked
prop이 DOM에 전달되지 않도록 해주신 거 맞나요?
이 기능은 처음 봤는데 DOM에 불필요한 props가 남지 않도록 관리할 수 있어서 좋은 것 같아요 ㅎㅎ
저도 props를 사용한 스타일링에서 DOM에 전달되지 않도록 shouldForwardProp
을 써봐야겠어요!
<GameBox $gridLength={gridLength}> | ||
{initialNums.map((number, index) => | ||
number !== null ? ( | ||
<NumberButton | ||
key={index} | ||
onClick={() => handleClick(number, index)} | ||
disabled={number < currentNumber} | ||
isClicked={clickedIndexes.has(index)} | ||
> | ||
{number} | ||
</NumberButton> | ||
) : ( | ||
<EmptySpace key={index} /> | ||
) | ||
)} |
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.
p3:
숫자가 없는 자리를 EmptySpace로 두어 빈 공간을 정리하는 방식이 너무 좋은 것 같아요!
저는 빈 공간을 따로 정의하지 않고 isNew라는 상태를 받아서 opacity로 셀을 안 보이게 했거든요.
EmptySpace 덕분에 그리드 레이아웃이 더 깔끔하게 유지되는 것 같아요! 👏 아이디어 얻어갑니다...
const GameBoard = ({ | ||
initialLength, | ||
remainingLength, | ||
gridLength, |
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.
p3:
gameSelectLevel로 아예 레벨 선택 로직을 유틸로 분리해두신 게 흥미롭네요,,!!
유틸 활용을 잘 하고 계셔서 코드가 더 재사용성이 높은 것 같아요!! 배워갑니다 ㅎㅎ
import ToggleButton from "./ToggleButton"; | ||
import SelecteLevel from "./SelecteLevel"; | ||
import TimerTime from "./TimerTime"; |
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.
p2:
저는 컴포넌트 분리에 있어서 헤더에 가장 고민이 많았는데요...!
이렇게 요소별로 세부 컴포넌트들을 분리하고 싶은데 그럼 상태 설계나 관리가 너무 복잡해지려나? 하는 생각에 컴포넌트 분리를 쉽게 시도하지 못했어요.
근데 단일 책임 원칙에 따라 세부 컴포넌트들을 깔끔하게 분리하신 걸 보니 많이 배워갑니다! 저도 한 번 시도해봐야겠어요 😊
✨ 구현 기능 명세
💡 기본 과제
18까지 클릭해야한다면, 처음에는 19까지의 숫자가 랜덤으로 보여짐현재 시각
,게임의 레벨
,플레이 시간
3개의 정보를 localStorage에 저장 (랭킹에서 사용)🔥 심화 과제
Level 1:
3 x 3
, Level 2:4 x 4
, Level 3:5 x 5
createPortal
공유과제
제목: [React] - 개발 단계에서 고민해보면 좋은 것들(상태, 컴포넌트)
링크 첨부 : https://wave-web.tistory.com/78
❗️ 내가 새로 알게 된 점
createPortal 이벤트 처리
createPortal
을 사용하면, 부모 컴포넌트 트리와는 독립적으로 페이지 상단에 표시된다. React의 이벤트 버블링은 원래 React 트리의 계층 구조를 유지하므로, 부모 컴포넌트의 이벤트 핸들러가 그대로 작동하지 않는거 아닌가?라는 의문이 생겼다.createPortal
을 통해서 렌더링된 컴포넌트는 DOM 트리에서는 부모 컴포넌트 트리와 독립적으로 존재하지만, React 트리에서는 여전히 원래 부모 컴포넌트의 자식으로 간주된다고 한다. 따라서 React의 이벤트 버블링은 React 트리를 기준으로 동작하므로,createPortal
을 통해 렌더링된 요소에서도 부모 컴포넌트의 이벤트 핸들러가 작동하게된다고 한다.예시 코드
위의 예시에서
CompleteModal
내의 요소를 클릭하면handleClick
이 호출되고"Parent clicked!"
가 출력된다.CompleteModal
의 콘텐츠가 실제DOM
트리에서는modal-root
에 있지만, React 트리의 계층 구조에서는ParentComponent
의 자식으로 간주되기 때문이라고 한다.key 속성을 사용하여 강제로 초기화를 트리거
처음에는
useEffect
를 사용하여initialLength
,remainingLength
,gridLength
가 변경될 때마다resetGame
을 호출하여,level
변경 시GameBoard
가 새로운 숫자 배열과 설정으로 초기화할 수 있도록 구현했었다.하지만,
useEffect
를 지양을 넘어서 사용하지 말라는 멘토님들의 조언과useEffect
를 사용하지않고level
에 따라 랜더링 해줄 수 있는 방법이 있을 것 같아서 알아보았다.리액트 공식문서 중
useState
챕터를 읽어보던 중 아래 캡처 내용과 같이 컴포넌트에 다른key
를 전달하여 컴포넌트의state
를 초기화할 수 있다는 사실을 알게 되었다.위의 코드와 같이
key={level}
속성을GameBoard
에 추가하여level
이 변경될 때마다GameBoard
가 재생성될 수 있도록 할 수 있었다.❓ 구현 과정에서의 어려웠던/고민했던 부분
컴포넌트 분리
단일 책임 원칙
을 최대한 고려하여 컴포넌트 설계 하였습니다. 또한,관심사 분리
를 고려한 컴포넌트 분리를 진행하였습니다.CompleteModal.jsx
사용
모달 구현에서도 고민이 있었습니다. 지금은 간단한 과제이지만, 만약 프로젝트에서 이와 비슷한 모달창을 구현해야 한다면 어떻게 구현할까?를 고민했습니다. 저는 위와 같이
게임 기록
과닫기 버튼
을chilldren
으로 전달하여 사용할 수 있게 구현 하였습니다.이유
CompleteModal
컴포넌트가 변경에 유연하게 하기 위해서입니다. 현재는게임 기록
과닫기 버튼
만 모달 컨텐츠로 포함 되어있지만, 추후 변경에 유연하게 대처하기 위해서 children을 활용하였습니다.CompleteModal
컴포넌트가 모달 역할만 가지도록 하고 싶었습니다. 의존성 감소를 통해서 콘텐츠를 수정하거나 확장할 때 모달 컴포넌트 자체를 수정할 필요가 없어지게 됩니다. 즉, 유연성과 재사용성을 고려하였습니다.GameBoard
GameBoard
의 경우에도NextNumber
컴포넌트 단순히 다음에 클릭해야 하는 숫자를 UI에 보여주는 역할만하므로관심사 분리
차원에서 컴포넌트 분리하여 사용했습니다.상태 관리
Home.jsx
를 최상위 컴포넌트로 "어떤 상태를 최상위 컴포넌트에서 관리를 해야할까?"에 대한 고민이 있었습니다.크게 보면
Home
컴포넌트를 기준으로Header
와GameBoard
와RankingBoard
로 나뉘게 되는데,Header
컴포넌트에 존재하는 게임과 랭킹을 선택하는토글 버튼 상태
,레벨 선택 상태
와타이머 관련상태
는 여러 컴포넌트에서 공유되는 상태였습니다.그래서, 위와 같이
Home
컴포넌트(최상위컴포넌트)에서 상태를 관리하도록 설계하였습니다.궁금한 부분
🥲 소요 시간
12h
🖼️ 구현 결과물
헤더
2024-11-05.4.17.07.mov
게임, 모달, 레벨 선택
2024-11-05.4.21.48.mov
숫자 항상 랜덤, 타이머
2024-11-05.4.25.13.mov
로컬스토리지, 초기화 버튼
2024-11-05.4.27.58.mov