-
Notifications
You must be signed in to change notification settings - Fork 45
chore(CLNP-2919): fixed issues from message threading QA #182
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
Conversation
…t/message-threading
QA수정 내용 완료되어 다시 요청드립니다 @bang9 |
packages/uikit-react-native/src/components/GroupChannelMessageRenderer/index.tsx
Outdated
Show resolved
Hide resolved
packages/uikit-react-native/src/components/ChatFlatList/index.tsx
Outdated
Show resolved
Hide resolved
...kit-react-native/src/components/GroupChannelMessageRenderer/GroupChannelMessageReplyInfo.tsx
Outdated
Show resolved
Hide resolved
packages/uikit-react-native/src/components/ChannelInput/SendInput.tsx
Outdated
Show resolved
Hide resolved
packages/uikit-react-native/src/domain/groupChannel/component/GroupChannelMessageList.tsx
Outdated
Show resolved
Hide resolved
packages/uikit-react-native/src/fragments/createGroupChannelFragment.tsx
Outdated
Show resolved
Hide resolved
return '0 B'; | ||
} | ||
const units = ['B', 'KB', 'MB', 'GB', 'TB']; | ||
const digitGroups = Math.floor(Math.log10(fileSize) / Math.log10(1024)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
{headerTitle} | ||
</Text> | ||
{renderSubtitle()} | ||
<PressBox onPress={onPressHeader}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expected to navigate back when the header title area is pressed, instead of using the back button? What behavior was the QA team expecting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QA team expects navigation to go back when either the back button or the header title is pressed. Also, the behavior on other platforms is consistent with that the QA team expects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation. Do you happen to know the reason why? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood the behavior incorrectly.
When the back button is pressed, it navigates back.
When the subtitle is pressed, it navigates to the parent message.
I will fix the issue. @bang9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
scrollToMessageWithCreatedAt(props.searchItem.startingPoint, false, MESSAGE_SEARCH_SAFE_SCROLL_DELAY); | ||
} | ||
}, [isFirstMount]); | ||
}, [isChangedSearchItem]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 하나 고려를 해봐야 할 게 있는데요, 아래처럼 searchItem 이 inline object 로 전달될 경우
렌더링마다 새로운 오브젝트가 생성 되어 전달됩니다.
<GroupChannelFragment
searchItem={{ startingPoint: 12345678 }}
/>
이렇게 전달된 케이스에서는 렌더링마다 새로운 오브젝트가 생성되면서 (주소)값이 변했다고 인식되기 때문에
매 렌더링마다 side effect(useEffect 의 로직) 가 트리거 됩니다. (isFirstMount 를 추가해놓은 이유이기도 합니다.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그럼 searchItem 말고 searchItem.startingPoint 기준으로 처리하는건 어떨까요?
기능상 searchItem에 변화가 되었을때 스크롤이 되어야 하는데 현재는 startingPoint만 가지고 있어서 searchItem.startingPoint 이걸로 처리하고 추후에 종류가 늘어나면 다시 고려해보는것도...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옙 기존 로직에서 deps 에 [isFirstMount, props.searchItem?.startingPoint]
로만 업데이트 해서 적용 되는지 체크한번 해봐주실 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 로직에서 if 조건에 isFirstMount 가있어서 GroupChannel -> Thread -> GroupChannel 로 넘어갈경우 isFirstMount 가 false라서 실행이 안되는데요
현재 Thread -> GroupChannel 가는 상황에서 isFirstMount 가 false 인게 문제라 어떻게 수정하든 isFirstMount 관련 처리는 빠져야 할것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 그러네유! 넵 일단 빼시죠
For Internal Contributors
CLNP-2919
Description Of Changes