-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Chat): add different chat item types #255
Conversation
Codecov Report
@@ Coverage Diff @@
## master #255 +/- ##
=========================================
Coverage ? 91.69%
=========================================
Files ? 62
Lines ? 1168
Branches ? 150
=========================================
Hits ? 1071
Misses ? 94
Partials ? 3
Continue to review full report at Codecov.
|
From those options that are provided would rather vote against the third one, due to that it opens way to (accidentally) do things improperly on the consumer's side: <ChatMessage type='divider' bubble={{author: 'John Doe', ...}} ... /> Speaking of the rest two (Proposal 1 and Proposal 2) - the safest approach would be to start with just different type values being supported (no shorthands for now), and see how it will go afterwards; although I should notice that I do really like a shorthand-based approach (i.e. Proposal 2) as well 👍 |
Got it, I agree it's the easiest from user perspective as well. Will start with the Proposal 1, we can easily migrate to 2 if we see it would be needed. |
Actually, let's go directly with the Proposal 2, because even now I can see some problems with allowing the user to define different types for the Divider for example. I would like to avoid having to change the whole API rather soon... |
I'd go also with the second proposal. |
Not really in this moment, as the messages are collection shorthand inside the chat component, so we aren't really capable of adding some other component in between.. |
To be more specific the chat definition would look something like this: <Chat items={[
{{bubble: {avatar: {...}, content: '...'}}},
{{bubble: {avatar: {...}, content: '...'}}},
{{control: {icon: {...}, content: 'Meeting started...'}}}, //now that I think about this, maybe the better naming would be action
{{divider: {content: 'Today'}}},
{{bubble: {avatar: {...}, content: '...'}}},
]} So the general change is that, the chat messages will be transformed to chat items, which can be messages, dividers, action etc.. |
-renamed chat message to chat item component
Still work in progress, but this is how it currently looks: And here is the code for that example: import React from 'react'
import { Chat } from '@stardust-ui/react'
const janeAvatar = {
src: 'public/images/avatar/small/ade.jpg',
status: {
color: 'green',
icon: 'check',
title: 'Available',
},
}
const items = [
{
key: 1,
bubble: { content: 'Hello', author: 'John Doe', timestamp: 'Yesterday, 10:15 PM', mine: true },
},
{
key: 2,
bubble: {
content: 'Hi',
author: 'Jane Doe',
timestamp: 'Yesterday, 10:15 PM',
avatar: janeAvatar,
},
},
{
key: 3,
bubble: {
content: 'Would you like to grab a lunch?',
author: 'John Doe',
timestamp: 'Yesterday, 10:16 PM',
mine: true,
},
},
{
key: 4,
bubble: {
content: "Sure! Let's try the new place downtown",
author: 'Jane Doe',
timestamp: 'Yesterday, 10:16 PM',
avatar: janeAvatar,
},
},
{ key: 5, divider: { content: 'Today', type: 'primary', important: 'true' } },
{
key: 6,
bubble: {
content: "Let's have a call",
author: 'John Doe',
timestamp: 'Today, 11:15 PM',
mine: true,
},
},
{
key: 7,
action: { icon: 'record', content: 'Meeting started', timestamp: 'Today, 11:15 PM' },
},
]
const ChatExampleShorthand = () => <Chat items={items} />
export default ChatExampleShorthand @kuzhelov @alinais please give me your feedback about the API. |
@@ -1,11 +1,9 @@ | |||
import React from 'react' | |||
import Types from './Types' | |||
import Variations from './Variations' | |||
|
|||
const MessageExamples = () => ( |
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 know it's out of scope of PR, but we might want to rename to ChatExamples
to avoid confusion
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.
Sure, I will restructure to example containing all different chat items. Will consider this as well.
src/components/Chat/ChatItem.tsx
Outdated
static propTypes = { | ||
as: customPropTypes.as, | ||
|
||
/** Shorthand for the bubble message. */ |
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.
Since bubble
, divider
and action
props are mutually exclusive, we need a better way to prevent users try more than one at a time; I'd suggest something like this:
/** Shorthand for the bubble message. */
bubble: customPropTypes.disallow(['action', 'divider']),
/** Shorthand for the divider message. */
divider: customPropTypes.disallow(['action', 'bubble']),
/** Shorthand for the control message. */
action: customPropTypes.disallow(['bubble', 'divider']),
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.
Just the thing I was missing. Thanks a lot for the suggestion Alex!
src/components/Chat/ChatAction.tsx
Outdated
}: IRenderResultConfig<IChatActionProps>) { | ||
const { as, icon, children } = this.props | ||
|
||
return childrenExist(children) ? ( |
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.
@mnajdova - it looks like the Chat.Action
subcomponent is more complex (per @codepretty 's comments in our chats, we can have an array of actions with more complicated logic on what's rendered); I'm not even sure it falls under Stardust principles, maybe we should be more generic with the action
prop in Chat
component
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.
In my opinion, we shouldn't alter this component at this moment. We can reason for this in the following manner:
- Chat.Action can have children which can define whatever the user wants inside - in this case we won't change anything right now
- We can add actions as a list and make the Chat.Action use the Accordion component, but in that case it would be in my opinion very much Team's specific rather then common web (as @Bugaa92 said, it doesn't really fall under stardust principles)
My final thought on this is, leaving the Chat.Action component as is for this now, and address this later if we think would be necessary. First, I would like after this is merged, to try and address this in the chat pane and we will see how much complication it would bring.
-fixed style for the bubble when the content contains long words
-removed handled props from the Chat components
This is ready for final review @levithomason @kuzhelov @miroslavstastny |
src/components/Chat/ChatItem.tsx
Outdated
ElementType, | ||
classes, | ||
rest, | ||
styles, |
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.
nit: seems that these two guys could be omitted, as those are not used anywhere
src/components/Chat/ChatItem.tsx
Outdated
const { children, content } = this.props | ||
|
||
return ( | ||
<ElementType {...rest} className={cx(classes.root, classes.content)}> |
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.
we have potentially unreliable styling logic here - could you, please, suggest what are our plans to address it? Would rather expect this merge being performed in styles file, and exposed as item
styles then
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.
This is a wrong copy/paste error, thanks for noticing. Actually we will have just the classes.root here.
static displayName = 'ChatItem' | ||
|
||
static propTypes = { | ||
as: customPropTypes.as, |
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.
nit: description for this prop is missing (as well s for a few other components of this PR, like ChatMessage
) - is it intentional?
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.
It's not intentional, will provide description for all prop types in the components added/changed in this PR.
@@ -0,0 +1,52 @@ | |||
import React from 'react' |
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.
shorthand API currently is not working correctly in docs examples - the problem is apparent if one would try to click on 'Shorthand API' button:
Do we really want to dismiss shorthand case for Chat
? This seems to be confusing, as by looking on Chat
's implementation it is clearly visible that items
prop is exposed - thus, it is possible to utilize shorthand advantages for this component. More than that, it seems that we won't be able to get rid of Shorthand API support, due to in this case there won't be clear mechanisms for handling Chat
component's accessibility concerns.
@@ -18,6 +18,9 @@ const chatMessageStyles: IComponentPartStylesInput<IChatMessageProps, IChatMessa | |||
marginRight: 'auto', | |||
}), | |||
maxWidth: v.messageWidth, | |||
width: 'max-content', |
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.
could you, please, suggest what would be the styling problem if this CSS property won't be applied? The reason I am asking is because this one is marked as experimental: https://developer.mozilla.org/en-US/docs/Web/CSS/width
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.
src/components/Chat/Chat.tsx
Outdated
/** Shorthand array of messages. */ | ||
messages: PropTypes.arrayOf(PropTypes.any), | ||
/** Shorthand array of the items inside the chat. */ | ||
items: PropTypes.arrayOf(PropTypes.any), |
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.
nit: seems that we could be more specific here:
items: PropTypes.arrayOf(customPropTypes.itemShorthand)
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.
overall the vector of changes taken looks good to me 👍 - few moments that have raised my concerns, left comments for them.
… the chat message, provided description of the properties)
/> | ||
), | ||
}, | ||
<Chat.Item> |
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.
frankly, a bot unsure whether we should provide sort of heterogenous approach in the example - probably, it would be more intuitive if all the items will be the same (especially given that those that are at the end have different content - won't like to mix this differentiation aspect with another one that is about using Chat.Item
directly instead of content prop)
const { children, content } = this.props | ||
|
||
return ( | ||
<ElementType {...rest} className={cx(classes.root, classes.content)}> | ||
<ElementType {...rest} className={classes.root}> |
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.
👍
# Conflicts: # CHANGELOG.md
ChatMessage
This PR aims to add different types of messages for the chat. We should support the following types:
TODO
I have three proposals and would like to hear your opinion on.
API Proposal 1
type: 'bubble' | 'control' | 'divider'
The type property will be used for rendering one of the three different components. The properties will remain the same, we would just add additional icon shorthand for the control component.
Usage:
This would be easy for use, but not much open for fully customizing the generated component.
API Proposal 2
bubble: shorthand for the Message.Bubble component
control: shorthand for the Message.Control component
divider: shorthand for the Message.Divider component
Open for customization and consistent with the other usages of the shorthand. One problem is that, a message cannot be at the same time two of these types, so we will kind a have to decide behind the scene which would take precedence.
Usage
API Proposal 3
This proposal is combination of the previous two. We should use the type for deciding which component will be rendered, but will use the shorthand for generating the component itself.
type: 'bubble' | 'control' | 'divider'
bubble: shorthand for the Message.Bubble component
control: shorthand for the Message.Control component
divider: shorthand for the Message.Divider component