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

Fix logic about props 'emails' #132

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Fix logic about props 'emails' #132

merged 3 commits into from
Jul 17, 2023

Conversation

HyeongSeoku
Copy link
Contributor

@HyeongSeoku HyeongSeoku commented Jul 16, 2023

props로 들어가는 emails에 대한 로직 변경 의견

@thomasJang 해당 로직으로 돌려도 돌아갈 것 같은데 검증 부탁드리겠습니다.!

결국 내부 emails state에 props로 들어오는 emails가 세팅이 되어야해서 해당 useEffect 구문이 있는것 같은데
그렇다면 초기 useState값에 세팅이 되어도 기존 로직과 동일하게 동작하지 않을까 싶어서 의견 드립니다

  • 추가로 props로 들어오는 email에 대해서도 validation check가 필요할 것 같아서 initialEmailAddress를 추가했습니다.

@vercel
Copy link

vercel bot commented Jul 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-multi-email ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2023 5:36am

@thomasJang
Copy link
Member

처음 한번은 문제 없을 수 있을 거 같구요.
하지만 props을 외부에서 변경하는 경우엔 문제가 있을 수 있습니다.

https://axframe-datagrid.vercel.app/propsChange
이런식의 페이지를 만들고 테스트 해보시면 좋을 것 같습니다.

@HyeongSeoku
Copy link
Contributor Author

HyeongSeoku commented Jul 16, 2023

Test Code

1. onChange 없이 state만 전달할 경우

function CustomizeStyleExample(props: Props) {
  const [emails, setEmails] = React.useState<string[]>(['email', 'test', '[email protected]']);

  return (
    <Container>
      <form>
        <h3>Email</h3>
        <ReactMultiEmail
          delimiter={'[ ,;]'}
          placeholder={
            <>
              <b>I</b> am <u style={{ color: '#1890ff' }}>placeholder</u> !
            </>
          }
          style={{ minHeight: '100px' }}
          emails={emails}
          // onChange={(_emails: string[]) => {
          //   setEmails(_emails);
          // }}
          getLabel={(email, index, removeEmail) => {
            return (
              <div data-tag key={index}>
                <div data-tag-item>{email}</div>
                <span data-tag-handle onClick={() => removeEmail(index)}>
                  ×
                </span>
              </div>
            );
          }}
        />
        <br />
        <h4>react-multi-email value</h4>
        <p>{emails.join(', ') || 'empty'}</p>
      </form>
    </Container>
  );
}

Kapture 2023-07-16 at 15 09 08

@HyeongSeoku
Copy link
Contributor Author

HyeongSeoku commented Jul 16, 2023

2. onChange 도 있을 경우

function CustomizeStyleExample(props: Props) {
  const [emails, setEmails] = React.useState<string[]>(['email', 'test', '[email protected]']);

  return (
    <Container>
      <form>
        <h3>Email</h3>
        <ReactMultiEmail
          delimiter={'[ ,;]'}
          placeholder={
            <>
              <b>I</b> am <u style={{ color: '#1890ff' }}>placeholder</u> !
            </>
          }
          style={{ minHeight: '100px' }}
          emails={emails}
           onChange={(_emails: string[]) => {
             setEmails(_emails);
           }}
          getLabel={(email, index, removeEmail) => {
            return (
              <div data-tag key={index}>
                <div data-tag-item>{email}</div>
                <span data-tag-handle onClick={() => removeEmail(index)}>
                  ×
                </span>
              </div>
            );
          }}
        />
        <br />
        <h4>react-multi-email value</h4>
        <p>{emails.join(', ') || 'empty'}</p>
      </form>
    </Container>
  );
}

Kapture 2023-07-16 at 15 24 05

} = props;
const emailInputRef = React.useRef<HTMLInputElement>(null);

const [focused, setFocused] = React.useState(false);
const [emails, setEmails] = React.useState<string[]>([]);
const [emails, setEmails] = React.useState<string[]>(() => initialEmailAddress(propsEmails));
Copy link
Member

Choose a reason for hiding this comment

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

이런식도 가능 했네요. state를 많이 안쓰다보니. 오늘 하나 배워 갑니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

추가로 덧붙이자면..
얼마전까지 제가 많이 실수하던 코드인데

// 1번 - BAD
const [selected, setSelected] = useState(getIsSelected(id));

// 2번 - GOOD
const [selected, setSelected] = useState(() => getIsSelected(id));

의도 자체는 랜더링시에 한번만 getIsSelected를 실행시키기 위한 코드지만 1번과 같이 짤경우 랜더링시마다 getIsSelected 를 실행하게 되어 getIsSelected로직이 복잡할 경우 성능에 영향을 줄 수 있다고 합니다.

@HyeongSeoku HyeongSeoku mentioned this pull request Jul 16, 2023
Copy link
Contributor

@hwonda hwonda left a comment

Choose a reason for hiding this comment

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

와.. 저도 많이 배워갑니다. 고생하셨습니다! 👍

@thomasJang thomasJang added the enhancement New feature or request label Jul 17, 2023
Copy link
Contributor

@vinyl810 vinyl810 left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다 🔥

@vinyl810 vinyl810 merged commit a2aea8b into master Jul 17, 2023
2 checks passed
@HyeongSeoku
Copy link
Contributor Author

#154

@HyeongSeoku HyeongSeoku mentioned this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants