feat(fuselage): MessageEmoji and ThreadMessageEmoji components #709
Conversation
| image, | ||
| children, | ||
| }: MessageEmojiBaseProps) => ( | ||
| <div |
There was a problem hiding this comment.
| <div | |
| <span |
Because it could be children of <p>
There was a problem hiding this comment.
Yeah, I changed it's display to be inline-block so this shouldn't cause problems, but I will change because it's more semantic. Thanks for the tip!
| 'rcx-message__emoji', | ||
| className, |
There was a problem hiding this comment.
| 'rcx-message__emoji', | |
| className, | |
| className && 'rcx-message__emoji', | |
| className, |
Can we add a conditional here? cause when the className is undefined we will render the emoji text like :random-text: so we don't need the emoji style in this scenario.
There was a problem hiding this comment.
also we need the name in the class name to render the correct style of emoji
There was a problem hiding this comment.
I kind of agree with the first comment. I know we need for this to be "sizeless" whenever the emoji doesn't exist, but not sure if we should depend on the className. For ex: if for some reason we decide to pass another className, this implementation falls apart. Maybe we need an aditional prop?
On the second one, I don't se a reason to add the name in the className, because that is Application specific. If the application needs to send the name as a className, it can do so by including it in the className prop.
There was a problem hiding this comment.
I just realized that for the first comment, we can just not use this component :p. I'll leave it like it is...
Proposed changes (including videos or screenshots)
Issue(s)
Further comments