Skip to content
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

[럼카] 2주차 1번째 PR #84

Merged
merged 8 commits into from
May 18, 2022

Conversation

yongseongjeon
Copy link
Collaborator

@yongseongjeon yongseongjeon commented May 18, 2022

에드님 안녕하세요.

리뷰 해주셔서 감사합니다.

현재 금액 충전 기능을 추가는 하였으나 개인적으로 코드의 구조가 맘에 들지 않아 리팩토링을 진행중입니다.

데모 페이지입니다.

진행 상황

1. React 관련
    . e.g. `react`, `react-router-dom`
2. styled-component
3. pages
4. components
5. utils
6. mocks
7. constants

동일한 order를 가지고 있을 경우에는 알파벳 오름차순으로 정렬한다.
curMoney, showErrorMsg는 함께 변하지 않는 상태라고 판단되어 Context를 나눔.
- 자판기에 충전된 돈에서 지갑에 남아있는 돈으로 변경
- 탭 이동시에도 코인의 상태가 유실되지 않는다.
(라우터 하위에 있던 상태를 App에서 생성 및 관리함.)
@yongseongjeon yongseongjeon added the review-FE New feature or request label May 18, 2022
@yongseongjeon yongseongjeon requested a review from sungik-choi May 18, 2022 10:03
Copy link

@sungik-choi sungik-choi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다! 코드가 점점 깔끔해지시는 거 같아요. 구현하신 부분도 잘 동작하는 거 같습니다.
이슈 정리해주시는 거, 커밋 나눠주시는 거 정말 좋네요 👍

다음 PR에선 기능도 좀 더 추가되면 좋을 거 같습니다! 구현하면서 느낀 고민들도 PR에 더 적어주셔도 좋고요.

const [isInputMode, setInputMode] = useState(false);
const UnmodifiableInput = React.useCallback(
() => getUnmodifiableInput({ value: curMoney, handler: handleInputMode }),
[],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[],
[curMoney, handleInputMode],

Copy link
Collaborator Author

@yongseongjeon yongseongjeon May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useCallback을 사용하기 이전에는 다음과 같이 컴포넌트가 중첩된 구조를 가졌습니다.

function ControlPanel() {
  function ModifiableInput() {
    return <div />;
  }
  function UnmodifiableInput() {
    return <div />;
  }

  return (
    <div>
      {isInputMode ? <ModifiableInput /> : <UnmodifiableInput />}
    </div>
  );
}

그래서 react/no-unstable-nested-components 관련 문서를 찾아보니

  1. 컴포넌트를 밖으로 분리
  2. useCallback을 활용해 메모이제이션해서 사용

을 통해서 중첩된 구조에 대한 문제를 해결할 수 있다고 이해했고, 그 중 2번을 선택해서 해결해보고 싶었습니다.

원래 <ModifiableInput /> 컴포넌트도 useCallback을 활용하고 싶었지만 상태가 변할때마다 focus가 사라지는 이슈때문에 컴포넌트를 밖으로 분리했습니다.

focus 사라지는 이슈 관련 영상입니다. (스크롤 조금만 내려주시면 관련 영상이 있습니다.)

2번 이유를 선택했던 이유는 <ControlPanel /> 에서만 <ModifiableInput />, <UnModifiableInput /> 를 사용하기 때문에 해당 컴포넌트 내부에서 정의하고 싶었습니다. 또한 components 하위에 외부 컴포넌트로 분리하게 된다면 다른 컴포넌트에서 사용될 수 있는 여지를 줄 것 같다고 생각했습니다.

그런데 말씀해주셔서 다시 보니 <ModifiableInput />, <UnModifiableInput /> 두 컴포넌트가 1, 2번을 혼용해서 사용하고 있어서 코드의 일관성이 떨어져 보이는 면도 있는 것 같습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useCallback 으로 함수형 컴포넌트를 메모이제이션하려는 건 안티패턴으로 보여요. 또한 useCallback 은 함수를 메모이제이션하기때문에 함수의 재생성을 막을 뿐이지, 함수의 결과값을 메모이제이션하지 않아서 메모이제이션의 의미도 크지 않아 보입니다.

만들어주신 getUnmodifiableInput 함수는 결국 리액트 함수형 컴포넌트로 치환할 수 있어요. 컴포넌트(정확히는 함수형 컴포넌트의 결과값)를 메모이제이션하고 싶으신거라면, React.memo 를 한 번 공부해보시면 좋을 거 같습니다!

Comment on lines +3 to +7
const UNITS_MONEY = [10000, 5000, 1000, 500, 100, 50, 10];
const COINS = UNITS_MONEY.map((unit) => ({
AMOUNT: unit,
CNT: getRandomNumber({ min: 0, max: 10 }),
}));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 훨씬 깔끔해졌네요

Comment on lines +3 to +7

function ModifiableInput({ value, handler }) {
return (
<>
<Input as="input" type="number" onChange={handler} value={value} />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원래 input 엘리먼트라(styled.input), as="input" 은 불필요해보이네요.

));
function handleCoinCount(coinIdx) {
const newCoins = coins.map((coin, idx) => {
const isTargetCoin = idx === coinIdx;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인덱스 대신 코인 금액으로 비교하는 편이 더 직관적이어서 좋지 않을까요?

Copy link
Collaborator Author

@yongseongjeon yongseongjeon May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왜 인덱스로 비교했는지 모르겠을 정도로 코인 금액으로 비교하는 것이 훨씬 더 직관적이고 좋네요..! 감사합니다.

아래와 같이 수정했습니다.

function updateCoinCnt(coin) {
  const isTargetCoin = AMOUNT === coin.AMOUNT;
  if (isTargetCoin) {
    return { AMOUNT, CNT: CNT - 1 };
  }
  return coin;
}

@sungik-choi sungik-choi merged commit 72f2061 into codesquad-members-2022:rumka May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-FE New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants