refactor: vaniilaJS -> React#23
Merged
yongchanson merged 1 commit intoreact-migrationfrom Jun 11, 2022
Merged
Conversation
Member
chaerin-dev
commented
Jun 10, 2022
- 상태관리 없이 구현하는 것이 더 어려워서 Recoil을 이용한 전역상태관리까지 같이 구현하게 되었습니다.
- React 공부를 따로 할 시간이 없어 지난 미팅 때 @codeisneverodd 님께서 설명해주신 내용과 구글링만을 바탕으로 구현하다보니 부족한 점이 많습니다. 리뷰 부탁드립니다.
- CSS의 경우 Styled Component로 다시 구현하신다고 하셔서 아예 import 하지 않았습니다.
- @yongchanson CSS 적용하실 때 로딩컴포넌트 position을 absolute로 해서 나머지 컴포넌트들을 덮도록 해주시면 될 것 같습니다.
Member
codeisneverodd
left a comment
There was a problem hiding this comment.
좋은 코드 잘 보았습니다! 고생 많이셨을 것 같네요 👍
리뷰 남겼으니 답글을 남기셔도 좋고, 반영을하셔도 좋을 것 같습니다!!
Comment on lines
+6
to
+23
| export const solutionState = atom({ | ||
| key: "solutionState", | ||
| default: { | ||
| level: 1, | ||
| fileName: "", | ||
| solution: [], | ||
| }, | ||
| }); | ||
|
|
||
| const root = ReactDOM.createRoot(document.getElementById('root')); | ||
| export const solutionNoState = atom({ | ||
| key: "solutionNoState", | ||
| default: 0, | ||
| }); | ||
|
|
||
| export const loadingState = atom({ | ||
| key: "loadingState", | ||
| default: true, | ||
| }); |
Member
There was a problem hiding this comment.
atom.js 라는 파일을 새롭게 만들어 관리하면 좋을 것 같네요!
| ).join('')} | ||
| `; | ||
| export default function SearchableList() { | ||
| let [fileListHTML, changeState] = useState(""); |
Member
There was a problem hiding this comment.
상태는 직접 변동이 불가능하고 set함수로만 변경할 수 있기 때문에 const로 선언하는 것이 좋아보입니다! 그리고 state가 여러개가되면 이름의 혼동이 있을 수 있기 때문에 다음과 같이 수정하는 것이 좋아보입니다.
const [fileListHTML, setFileListHTML] = useState("")| const setLoadingState = useSetRecoilState(loadingState); | ||
|
|
||
| // TODO: ``부분 수정 필요.. | ||
| (async function fillList() { |
Member
There was a problem hiding this comment.
fillList에 대한 로직이 개선되어야할 것으로 보입니다! 우선 기존 로직을 이용해서 잘 구현해주셨다고 생각합니다. 하지만 상태 값이 생겼기 때문에 이제는 직접 돔을 채워주는 것이 아닌, 상태값을 이용한 형태로 구현이 가능할 것 같습니다.
예시를 적어보자면 아래와 같습니다.
const POSSIBLE_LEVELS = [1, 2, 3, 4, 5];
const [fileList, setFileList] = useState({})
useEffect(()=>{
const newFileList = Object.assign({},fileList)
for (const level of POSSIBLE_LEVELS) {
newFileList[level] = await getFileList(level);
}
setFileList(newFileList)
},[ ])
// 돔 내부
{Object.entries(fileList).map(([level, files])=>{
//로직 작성
})}
Member
There was a problem hiding this comment.
GitHub을 통해 작성한 것이라 오류가 있을 수 있습니다! 참고용으로만 보아주세요 ㅎㅎ
Member
Author
|
@codeisneverodd 👍👍👍 리뷰 남겨주신 부분 반영해보도록 하겠습니다! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.