Skip to content

Conversation

@dinohan
Copy link
Member

@dinohan dinohan commented Oct 11, 2022

Self Checklist

  • I wrote a PR title in English.
  • I added an appropriate label to the PR.
  • I wrote a commit message in English.
  • I wrote a commit message according to the Conventional Commits specification.
  • I added the appropriate changeset for the changes.
  • I wrote a unit test about the implementation.
  • I wrote a storybook document about the implementation.
  • I tested the implementation in various browsers.
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox

Summary

  • FormLabelHelp 컴포넌트를 추가합니다.
  • FormLabel의 help prop을 deprecate합니다.
  • FormLabel에 HelpTooltip prop을 추가합니다.
    • FormLabel의 HelpTooltip을 통해 FormLabelHelp 컴포넌트를 합성 합니다.

Details

기존 FormLabel에선 help만 받았고, 이는 Tooltip의 content에만 영향을 줬습니다.
Tooltip의 다른 prop (delayHide 등) 을 사용해야 하는 상황이 있어,
컴포넌트 합성을 통해 FormLabel Help 컴포넌트의 사용성을 개선합니다.

Breaking change or not (Yes/No)

References

@dinohan dinohan self-assigned this Oct 11, 2022
@changeset-bot
Copy link

changeset-bot bot commented Oct 11, 2022

🦋 Changeset detected

Latest commit: 85f37ad

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@channel.io/bezier-react Minor
bezier-figma-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

Chromatic Report

🚀 Congratulations! Your build was successful!

@dinohan
Copy link
Member Author

dinohan commented Oct 12, 2022

컴포넌트 복붙해서 작성했더니 대충 들어간 것들이 많았군요... 죄송..

@sungik-choi
Copy link
Contributor

sungik-choi commented Oct 12, 2022

유즈 케이스들을 보면 이 물음표 아이콘 + 툴팁 컴포넌트가 폼 라벨에 사용되는 것 이외에도 많이 사용되어서, FormLabelHelp 가 아니라 Help 등으로 이름지어져도 괜찮지 않을까 싶네요(Forms 디렉토리 외부로 이동 및 스토리 작성). 컴포넌트의 디테일한 사용 방식을 디자인팀에게 확인해보면 좋을 거 같습니다.

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Base: 71.00% // Head: 70.84% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (2fbd218) compared to base (6675246).
Patch coverage: 57.14% of modified lines in pull request are covered.

❗ Current head 2fbd218 differs from pull request most recent head 85f37ad. Consider uploading reports for the commit 85f37ad to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           next-v1     #932      +/-   ##
===========================================
- Coverage    71.00%   70.84%   -0.16%     
===========================================
  Files          204      209       +5     
  Lines         2894     2909      +15     
  Branches       807      810       +3     
===========================================
+ Hits          2055     2061       +6     
- Misses         721      729       +8     
- Partials       118      119       +1     
Impacted Files Coverage Δ
...src/components/Forms/FormLabel/FormLabel.styled.ts 100.00% <ø> (ø)
...ts/Forms/FormLabel/FormLabelHelp/FormLabelHelp.tsx 0.00% <0.00%> (ø)
...FormLabel/FormLabelHelp/useFormLabelHelpContext.ts 0.00% <0.00%> (ø)
...react/src/components/Forms/FormLabel/FormLabel.tsx 88.88% <71.42%> (-11.12%) ⬇️
...ms/FormLabel/FormLabelHelp/FormLabelHelpContext.ts 100.00% <100.00%> (ø)
...es/bezier-react/src/components/Help/Help.styled.ts 100.00% <100.00%> (ø)
packages/bezier-react/src/components/Help/Help.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dinohan dinohan requested a review from sungik-choi October 12, 2022 10:43

if (React.isValidElement(help)) { return help }
if (React.isValidElement(help) &&
get(help, 'type.displayName') === HELP_DISPLAY_NAME) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 이렇게 display name을 체크했을 때, styled component로 감싸거나, 컴포넌트를 합성해서 넣으면 체크가 fail이기 때문에 어려움이 있던 경험이 있습니다.

데스크의 FlatTable에서 자주 발생하는 문제점이죵

Copy link
Contributor

Choose a reason for hiding this comment

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

FormControl 의 구현처럼, displayName 을 통한 체크 대신 컨텍스트를 통해 처리하면 위 같은 케이스도 커버할 수 있을 거 같아요.

상위 컴포넌트(여기선 FormLabel)에서 컨텍스트 값으로 boolean 상태에 대한 setter를 제공하고, Help 컴포넌트 내부에선 해당 컨텍스트를 참조해서 마운트됐을 때 해당 플래그를 참으로 변경하면 될 거 같습니다. FormGroup 컴포넌트와 FormControl 컴포넌트가 커뮤니케이션하는 방식을 참고해주시면 좋을 거 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

Help 컴포넌트 재사용이 어려울 것 같아요.
Help 컴포넌트를 재사용하려면 FormLabelHelp 같은 컴포넌트를 하나 더 만들어서 여기서만 useEffect로 마운트를 관리하게 해야 할 것 같네요.

Copy link
Contributor

Choose a reason for hiding this comment

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

음... 고민되네요. 재사용과 큰 연관은 없다고 생각하는데, Textfield 같은 폼 컴포넌트들이 FormControl 없이도 동작하는 것과 같은 이유에서입니다. 불필요한 이펙트를 실행하긴 하나 옵셔널 체이닝을 사용하면 별다른 오버헤드 없이 라벨 이외의 사용처에서도 재사용할 수 있지 않을까 싶었어요.

Copy link
Member Author

@dinohan dinohan Oct 13, 2022

Choose a reason for hiding this comment

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

Help 하나만 있어서는 FormLabel에서의 Help 컴포넌트 재사용이 어렵다는 의미였습니다.
help가 string이면 Help로 감싸고, Help 컴포넌트에서 isRendered를 넣어준다고 해볼게요.
(작성 중)

Copy link
Contributor

@sungik-choi sungik-choi Oct 13, 2022

Choose a reason for hiding this comment

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

help prop에 대한 string 타입체크를 먼저 수행하면 상관 없지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

string은 예시이고, Fragment나 Portal 같은 다른 형태여도 마찬가지 입니다.

Copy link
Member Author

@dinohan dinohan Oct 13, 2022

Choose a reason for hiding this comment

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

  const HelpComponent = useMemo(() => {
    if (isEmpty(help)) { return null }

    if (isHelpRendered) {
      return help
    }

    return (
      <Help>
        { help }
      </Help>
    )
  }, [
    help,
    isHelpRendered,
  ])

isHelpRendered 는 context를 통해 Help 컴포넌트에서 set 해주는 값이라고 할 때,
help에 string이나 Fragment, 또는 아무 Element 등 Help가 아닌 다른 노드가 온다면 첫 렌더에서는 isHelpRendered가 false입니다.
그런데 help 통과해서 <Help> { help } </Help> 를 return 하고 렌더하는 순간 isHelpRendered가 true가 됩니다.
실제로 Help가 마운트 되었기 때문에용
그러면 return help를 하고 처음 마운트 된 <Help> { help } </Help>는 언마운트 되며 isHelpRendered가 또 false가 되어요.

Copy link
Member Author

Choose a reason for hiding this comment

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

근데 어쨌든 FormLabelHelp가 추가되는 방향이 괜찮은 것 같아서 문제 없습니다!ㅋㅋ

Copy link
Contributor

@sungik-choi sungik-choi Oct 13, 2022

Choose a reason for hiding this comment

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

아 저는 isEmpty 체크 대신 isString 체크를 맨 처음 하고, 맞다면 help로 wrapping 해서 미리 리턴하면 문제없을 거 같단 이야기였어요.


생각해보니 순서상 렌더 이후 마운트가 이루어지니, isRendered가 참이 되려면 일단 렌더가 완료되어야하는데, 그 전에 조건 체크를 하게 되네요. 고민 좀 더 해볼게요.

@dinohan dinohan changed the title Implement FormLabelHelp component for FormLabel HelpTooltip Implement Help component for FormLabel HelpTooltip Oct 13, 2022
@dinohan dinohan changed the title Implement Help component for FormLabel HelpTooltip Implement Help component for FormLabel help tooltip Oct 13, 2022
Copy link
Contributor

@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.

Help 에 대한 스토리와 DOM 테스트 코드 추가해주시면 👍


if (React.isValidElement(help)) { return help }
if (React.isValidElement(help) &&
get(help, 'type.displayName') === HELP_DISPLAY_NAME) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FormControl 의 구현처럼, displayName 을 통한 체크 대신 컨텍스트를 통해 처리하면 위 같은 케이스도 커버할 수 있을 거 같아요.

상위 컴포넌트(여기선 FormLabel)에서 컨텍스트 값으로 boolean 상태에 대한 setter를 제공하고, Help 컴포넌트 내부에선 해당 컨텍스트를 참조해서 마운트됐을 때 해당 플래그를 참으로 변경하면 될 거 같습니다. FormGroup 컴포넌트와 FormControl 컴포넌트가 커뮤니케이션하는 방식을 참고해주시면 좋을 거 같습니다.

@dinohan dinohan force-pushed the fixes/form-label-help branch 2 times, most recently from d50ef67 to 823c4be Compare October 13, 2022 14:23
@dinohan
Copy link
Member Author

dinohan commented Oct 13, 2022

FormLabel.test.tsx 에서 FormLabelHelp import해서 테스트 넣었기 때문에 다 포함되는 line일 텐데 이상하네요..
스크린샷 2022-10-13 23 36 03

@dinohan dinohan force-pushed the fixes/form-label-help branch from 2fbd218 to 85f37ad Compare October 14, 2022 02:35
@dinohan dinohan merged commit c84d1da into channel-io:next-v1 Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants