refactor: Add testid to the components in Channel #1089
Conversation
✅ Deploy Preview for sendbird-uikit-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
HoonBaek
force-pushed
the
refactor/CLNP-2666/Add-testid-to-channel-components
branch
from
f152fac
to
c5e31c2
Compare
src/modules/GroupChannel/components/FileViewer/FileViewerView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
혹시라도 기존에 사용하고 있던 고객이 있다면 문제가 생길 수 있지 않을까 싶어서 삭제하지 않았습니다
ref: https://sendbird.atlassian.net/browse/SBISSUE-15260
There was a problem hiding this comment.
흠 저런게 있군요, 그럼 deprecated 마크 해놓고 넘어가시죠!
There was a problem hiding this comment.
deprecation 한 뒤에 가능하면, <MenuItem dataSbId={...} /> 로 작성된 코드들도
내부에서는 <MenuItem testID={...} /> 사용하도록 바뀌면 좋을것 같습니다.
| ])} | ||
| isAnimated && 'sendbird-msg-hoc__animated', | ||
| )} | ||
| data-testid="sendbird-message-hoc" |
There was a problem hiding this comment.
nit: 새롭게 무언가를 추가할때는, 기존것을 그냥 따라가는것도 유지보수에 좋기는 하지만
기존에서 개선할 부분이 있는지에 대한 부분 또한 고려되면 좋은데요 (레거시를 탈피할 좋은 기회)
hoc 가 고차 컴포넌트를 의미하는 단어로 사용되었던게 맞을까요?
만약 그렇다면, 현재 컴포넌트의 위치에 사용하기 적합한 단어인지를 고려해보면 좋을 것 같습니다.
| @@ -69,7 +69,7 @@ type MessageInputProps = { | |||
| channel: GroupChannel | OpenChannel; | |||
| message?: UserMessage; | |||
| value?: null | string; | |||
| className?: string | string[]; | |||
| className?: string; | |||
| @@ -53,6 +53,7 @@ const ReactionButton = React.forwardRef((props: ReactionButtonProps, ref: Forwar | |||
| {...onClickHandler} | |||
| tabIndex={0} | |||
| data-sb-id={dataSbId} | |||
| data-testid={dataSbId} | |||
There was a problem hiding this comment.
nit: 이건 기존에 사용하고있는 dataSbId 가 좀 많아서 이해는 되는 부분인데, 아래의 관점에서 좋은 선택일지 나중에 고민해보시면 좋을것 같아서 코멘트 남깁니다.
- 작성된 testID 와 dataSbId 의 컨벤션 문제 (히스토리를 모르는 개발자가, 나중에 코드를 보았을때 추측을 하게되는 코드는 좋지 않다.
ex: 여긴 왜 다른데랑 다른형식으로 작성이 되어있지?) - 새롭게 추가되는 인터페이스에, 앞으로 제거할 레거시를 이어가는것이 좋은 선택인지 (
ex: 여러번의 메이저 버전을 넘어오는 과정에서 이러한 선택이 없었는지, 있었다면 다음 메이저 버전에서 개선을 했었는지, 개선을 했다면 어떤 방식으로 이 태스크를 챙겼는지 등.. 과거에서 되돌아 볼 수 있음)
|
Do you have a ticket for this? |
Added
data-testidto the UI components for making it easily to select them in the QE test