sendbird-uikit-react icon indicating copy to clipboard operation
sendbird-uikit-react copied to clipboard

fix: add channel (currentGroupChannel) to RenderMessageProps

Open jln-dk opened this issue 2 years ago • 3 comments

Description Of Changes

  • Added channel: GroupChannel to RenderMessageProps in order to able to render correct message status using newly exported utils function getOutgoingMessageState(channel, message)

Reason

The built-in <MessageContent /> component has access to the "channel" object (the current active group channel) and using this it can render correct message status right here. See code references:

  1. https://github.com/sendbird/sendbird-uikit-react/blob/main/src/smart-components/Channel/components/Message/index.tsx#L314
  2. https://github.com/sendbird/sendbird-uikit-react/blob/main/src/ui/MessageContent/index.tsx#L218
  3. https://github.com/sendbird/sendbird-uikit-react/blob/main/src/ui/MessageStatus/index.tsx#L33

But if you are using the function renderMessage?: (props: RenderMessageProps) => React.ReactElement on the <Channel /> component to override/extend the component behaviour, this function does not provide the channel object and therefore you are not able to render the correct message status. See code:

  1. https://github.com/sendbird/sendbird-uikit-react/blob/main/src/smart-components/Channel/index.tsx#L38
  2. https://github.com/sendbird/sendbird-uikit-react/blob/main/src/smart-components/Channel/components/Message/index.tsx#L35
  3. https://github.com/sendbird/sendbird-uikit-react/blob/main/src/smart-components/Channel/components/Message/index.tsx#L166)
  4. https://github.com/sendbird/sendbird-uikit-react/blob/main/src/types.d.ts#L38

Let me know if you have any questions.

Types Of Changes

  • [x] Bugfix
  • [ ] New feature
  • [ ] Documentation (correction or otherwise)
  • [ ] Cosmetics (whitespace, appearance (ex) Prettier)
  • [ ] Build configuration
  • [ ] Improvement (refactor code)

jln-dk avatar Oct 27 '22 07:10 jln-dk

Hello @jln-dk thanks for the PR. but, I think, you can use useChannelContext hook to access current channel.. Wouldnt this solve your requirement?

https://codesandbox.io/s/2-5-customizing-messageinput-forked-or4lm8?file=/src/CustomizedMessageInput.js:827-871 As in here ^^

sravan-s avatar Oct 27 '22 19:10 sravan-s

@sravan-s > That's strange... I just tried that the other day and ran into "Invalid Hook Call Warning" (https://reactjs.org/warnings/invalid-hook-call-warning.html). But now it seems to be working. Must have been something else then. Thanks!

jln-dk avatar Oct 28 '22 08:10 jln-dk

@sravan-s > On another note: Looking at the project source code it's clear that there are differences between which information/props that your own built-in UI components receive and which information/props that "outside" renderX functions (when overriding/extending the behaviour) have access to. This can lead to confusions, false issues (like this one) and potentially bugs.

If I may suggest you should split up your UI Kit in two parts:

  1. A set of naked "base components" that only handle all the logic. No styling at all. Only the primitives needed for building the chat app. All of these base components should expose the appropriate renderX methods (eg: renderChannelListItem, renderMessage etc.) so that everyone can have the same "naked" building blocks to start from.
  2. Now, next to this set of "base components", you should have your UI components for the default/built-in chat UI. The ones with all the styling etc. This way you do dogfooding of your own UI Kit's "base components" and make sure the API/render methods are correct and complete, while you also provide clear instructions and inspiration for other developers when looking into the project source code: like "How do Sendbird render this message? I can go have a look at their implementation and copy-paste the whole thing and tweak it to my needs".

I think this approach by separating "building blocks" from "out-of-the-box UI" will make the UI Kit much more scalable and future-proof.

Please let me know if this makes sense to you or not.

jln-dk avatar Oct 28 '22 09:10 jln-dk

Yeah, your ideas makes sense, we would also wanted to do it this way.. But there were some logistic constraints that we had to deal with. Lets see if we can refactor this way for the next "major" release 👍

sravan-s avatar Oct 31 '22 03:10 sravan-s