feat: show edited message label in message UI #2514
Conversation
package/package.json
Outdated
| @@ -78,7 +78,7 @@ | |||
| "path": "0.12.7", | |||
| "react-native-markdown-package": "1.8.2", | |||
| "react-native-url-polyfill": "^1.3.0", | |||
| "stream-chat": "8.17.0" | |||
| "stream-chat": "8.21.0" | |||
There was a problem hiding this comment.
Any reason why you aren't using the latest LLC?
There was a problem hiding this comment.
No reasons. We haven't tested it yet, and that is why didn't upgrade to the latest.
There was a problem hiding this comment.
I think when updating a dependency, we should always update to the latest version. We test it anyways, this way we can cover a wider version range.
There was a problem hiding this comment.
we usually update to the latest.. @khushal87 could you upgrade and quickly check.. nothing should break with a patch release update
There was a problem hiding this comment.
Updated to the latest version. I don't see any problem as such as of now.
| const MemoizedMessageEditedTimestamp = React.memo( | ||
| MessageEditedTimestampWithContext, | ||
| areEqual, | ||
| ) as typeof MessageEditedTimestampWithContext; |
There was a problem hiding this comment.
Are there any good reasons why we are memorizing this component? All I can see are troubles with stale data down the road.
There was a problem hiding this comment.
There are no reasons. This has been a pattern in our RN SDK for a long time, and it's just been followed here as well. This way, we don't pass props down the hierarchy but can utilize them from context(if present). These are things that have to be improved everywhere explicitly.
There was a problem hiding this comment.
I think excessive memoization creates more problems than benefits. I understand this is a legacy pattern we also have in our React SDK but we should slowly move away from it (at least for the new stuff). Otherwise, we keep increasing the technical dept and we are breaking the expectations of React components - re-render when a prop changes.
There was a problem hiding this comment.
Also I see a problem with this memoisation, what if the timestamp changes?
we can remove this memoisation all together, there seems to be nothing expensive in the render function that warrants for memoisation here
There was a problem hiding this comment.
I have removed it completely
package/src/components/Message/MessageSimple/MessageEditedTimestamp.tsx
Outdated
Show resolved
Hide resolved
| const MemoizedMessageTimestamp = React.memo( | ||
| MessageTimestampWithContext, | ||
| () => true, | ||
| ) as typeof MessageTimestampWithContext; |
There was a problem hiding this comment.
Also here, this component will never receive an update. Do we need the memorization on this level?
I'm not sure how expensive getDateString() is. At best, I'd useMemo it only.
There was a problem hiding this comment.
Done for the same reasons so that tDateTimeParser can be taken both as a prop or a context if the prop is unavailable here. 🙂
There was a problem hiding this comment.
I'm referring to () => true. This component will show the stale timestamp until it remounts. Updates are cheap, we shouldn't be memoizing aggressively on a component level.
There was a problem hiding this comment.
I have removed the memoization here completely.
|
🎉 This PR is included in version 5.30.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎯 Goal
The feature adds a new edited message label that can also be expanded with the click of a message.
PFA the video to see how it looks and works.
Screen.Recording.2024-05-14.at.3.44.21.PM.mov
I have made our date formatter function reusable and better in the PR as well. This removes code duplicacy from the SDK and is readable as well.
🛠 Implementation details
🎨 UI Changes
iOS
Android
🧪 Testing
☑️ Checklist
developbranch