feat(Chat): add different chat item types #255

mnajdova · 2018-09-20T09:54:45Z

ChatMessage

This PR aims to add different types of messages for the chat. We should support the following types:

control messages
bubble messages (this is currently the implementation of the ChatMessage itself)
divider messages

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:

<ChatMessage type='bubble' author={...} ... />
<ChatMessage type='control' icon={...} ... />
<ChatMessage type='divider' content={'Yesterday'} ... />

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

<ChatMessage bubble={{author: 'John Doe', ...}} ... />
<ChatMessage control={{icon: {{...}}, content='Some content...'}} ... />
<ChatMessage divider={{content: '.Yesterday}}... />

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

<ChatMessage type='bubble' bubble={{author: 'John Doe', ...}} ... />
<ChatMessage type='control'  control={{icon: {{...}}, content='Some content...'}} ... />
<ChatMessage type='divider' divider={{content: '.Yesterday}}... />

codecov · 2018-09-20T09:54:51Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ea881ee). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #255   +/-   ##
=========================================
  Coverage          ?   91.69%           
=========================================
  Files             ?       62           
  Lines             ?     1168           
  Branches          ?      150           
=========================================
  Hits              ?     1071           
  Misses            ?       94           
  Partials          ?        3

Impacted Files	Coverage Δ
src/components/Chat/ChatMessage.tsx	`93.61% <ø> (ø)`
src/components/Grid/Grid.tsx	`100% <ø> (ø)`
src/components/Chat/Chat.tsx	`95.45% <100%> (ø)`
src/index.ts	`100% <100%> (ø)`
src/components/Chat/ChatItem.tsx	`100% <100%> (ø)`
src/components/Chat/index.ts	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea881ee...e97beac. Read the comment docs.

kuzhelov · 2018-09-20T10:52:23Z

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 👍

mnajdova · 2018-09-20T12:12:36Z

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.

mnajdova · 2018-09-20T12:32:47Z

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...

alinais · 2018-09-20T12:43:44Z

I'd go also with the second proposal.
One thing though: why do we need the divider type of the message? Can't divider be simply a divider and used where needed?

mnajdova · 2018-09-20T12:58:04Z

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..

mnajdova · 2018-09-20T13:09:14Z

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

mnajdova · 2018-09-20T14:18:42Z

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.

bmdalex · 2018-09-21T10:20:07Z

docs/src/examples/components/Chat/index.tsx

@@ -1,11 +1,9 @@
 import React from 'react'
 import Types from './Types'
-import Variations from './Variations'

 const MessageExamples = () => (


I know it's out of scope of PR, but we might want to rename to ChatExamples to avoid confusion

Sure, I will restructure to example containing all different chat items. Will consider this as well.

bmdalex · 2018-09-21T10:26:41Z

src/components/Chat/ChatItem.tsx

+  static propTypes = {
+    as: customPropTypes.as,
+
+    /** Shorthand for the bubble message. */


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']),

Just the thing I was missing. Thanks a lot for the suggestion Alex!

src/components/Chat/ChatItem.tsx

src/components/Chat/ChatBubble.tsx

bmdalex · 2018-09-21T10:54:10Z

src/components/Chat/ChatAction.tsx

+  }: IRenderResultConfig<IChatActionProps>) {
+    const { as, icon, children } = this.props
+
+    return childrenExist(children) ? (


@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

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

mnajdova · 2018-09-27T15:37:10Z

This is ready for final review @levithomason @kuzhelov @miroslavstastny

kuzhelov · 2018-10-01T11:13:56Z

src/components/Chat/ChatItem.tsx

+    ElementType,
+    classes,
+    rest,
+    styles,


nit: seems that these two guys could be omitted, as those are not used anywhere

kuzhelov · 2018-10-01T11:16:06Z

src/components/Chat/ChatItem.tsx

+    const { children, content } = this.props
+
+    return (
+      <ElementType {...rest} className={cx(classes.root, classes.content)}>


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

This is a wrong copy/paste error, thanks for noticing. Actually we will have just the classes.root here.

kuzhelov · 2018-10-01T11:18:13Z

src/components/Chat/ChatItem.tsx

+  static displayName = 'ChatItem'
+
+  static propTypes = {
+    as: customPropTypes.as,


nit: description for this prop is missing (as well s for a few other components of this PR, like ChatMessage) - is it intentional?

It's not intentional, will provide description for all prop types in the components added/changed in this PR.

kuzhelov · 2018-10-01T11:22:18Z

docs/src/examples/components/Chat/Types/ChatExample.tsx

@@ -0,0 +1,52 @@
+import React from 'react'


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.

kuzhelov · 2018-10-01T11:46:46Z

src/themes/teams/components/Chat/chatMessageStyles.ts

@@ -18,6 +18,9 @@ const chatMessageStyles: IComponentPartStylesInput<IChatMessageProps, IChatMessa
          marginRight: 'auto',
        }),
    maxWidth: v.messageWidth,
+    width: 'max-content',


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

Without that prop this is how the chat looks:

I will try to implement this behavior using some other styling then...

kuzhelov · 2018-10-01T11:54:52Z

src/components/Chat/Chat.tsx

-    /** Shorthand array of messages. */
-    messages: PropTypes.arrayOf(PropTypes.any),
+    /** Shorthand array of the items inside the chat. */
+    items: PropTypes.arrayOf(PropTypes.any),


nit: seems that we could be more specific here:

items: PropTypes.arrayOf(customPropTypes.itemShorthand)

kuzhelov

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)

kuzhelov · 2018-10-01T13:31:29Z

docs/src/examples/components/Chat/Types/ChatExample.shorthand.tsx

+      />
+    ),
+  },
+  <Chat.Item>


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)

kuzhelov · 2018-10-01T13:31:57Z

src/components/Chat/ChatItem.tsx

    const { children, content } = this.props

    return (
-      <ElementType {...rest} className={cx(classes.root, classes.content)}>
+      <ElementType {...rest} className={classes.root}>


… for the Chat

# Conflicts: # CHANGELOG.md

-adjusting the header according to the styling guides

cab3db6

mnajdova requested review from dvdzkwsk, kuzhelov, levithomason and miroslavstastny as code owners September 20, 2018 09:54

mnajdova added RFC help wanted Extra attention is needed labels Sep 20, 2018

kuzhelov added the needs author feedback Author's opinion is asked label Sep 20, 2018

mnajdova added 🚧 WIP and removed help wanted Extra attention is needed needs author feedback Author's opinion is asked labels Sep 20, 2018

-added chat action, bubble components

53c4c4f

-renamed chat message to chat item component

-fixing linting errors

867e5d2

mnajdova changed the title ~~feat(Message): add different types~~ feat(Chat): add different chat item types Sep 20, 2018

manajdov added 3 commits September 20, 2018 17:07

-fixed example

fcc43fa

-fixed resizing of the bubble message

d4536b1

-added TODO for the wrapping

83e95ed

bmdalex reviewed Sep 21, 2018

View reviewed changes

-added restrictions on the bubble, divider and action props

8d66d14

-fixed style for the bubble when the content contains long words

manajdov added 4 commits September 27, 2018 14:26

-fixed changelog

c364df0

-removed handled props from the Chat components

-fixing merging conflicts

3def364

-fixes after merging

c4971c7

-fixes after merging

4c5a19f

docs: cleanup chat example

a7f5462

kuzhelov reviewed Oct 1, 2018

View reviewed changes

kuzhelov added needs author feedback Author's opinion is asked RFC and removed RFC labels Oct 1, 2018

-resolved comments on PR (added shorthand example, changed styles for…

b1ed360

… the chat message, provided description of the properties)

kuzhelov reviewed Oct 1, 2018

View reviewed changes

kuzhelov approved these changes Oct 1, 2018

View reviewed changes

manajdov added 3 commits October 1, 2018 15:45

-providede the content API for the ChatItems in the shorthand example…

e5b6e51

… for the Chat

Merge branch 'master' into feat/chat-message-types

18b025a

# Conflicts: # CHANGELOG.md

-merge conflicts and updating changelog

e97beac

mnajdova added ready for merge and removed needs author feedback Author's opinion is asked 🚀 ready for review labels Oct 1, 2018

mnajdova merged commit 3e5c4fb into master Oct 1, 2018

levithomason deleted the feat/chat-message-types branch October 11, 2018 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Chat): add different chat item types #255

feat(Chat): add different chat item types #255

mnajdova commented Sep 20, 2018 •

edited

codecov bot commented Sep 20, 2018 •

edited

kuzhelov commented Sep 20, 2018 •

edited

mnajdova commented Sep 20, 2018

mnajdova commented Sep 20, 2018

alinais commented Sep 20, 2018

mnajdova commented Sep 20, 2018

mnajdova commented Sep 20, 2018

mnajdova commented Sep 20, 2018 •

edited

bmdalex Sep 21, 2018

mnajdova Sep 21, 2018

bmdalex Sep 21, 2018

mnajdova Sep 21, 2018

bmdalex Sep 21, 2018

mnajdova Sep 21, 2018

mnajdova commented Sep 27, 2018

kuzhelov Oct 1, 2018

kuzhelov Oct 1, 2018 •

edited

mnajdova Oct 1, 2018

kuzhelov Oct 1, 2018 •

edited

mnajdova Oct 1, 2018

kuzhelov Oct 1, 2018 •

edited

kuzhelov Oct 1, 2018

mnajdova Oct 1, 2018 •

edited

kuzhelov Oct 1, 2018

kuzhelov left a comment

kuzhelov Oct 1, 2018

kuzhelov Oct 1, 2018

feat(Chat): add different chat item types #255

feat(Chat): add different chat item types #255

Conversation

mnajdova commented Sep 20, 2018 • edited

ChatMessage

TODO

API Proposal 1

type: 'bubble' | 'control' | 'divider'

Usage:

API Proposal 2

bubble: shorthand for the Message.Bubble component

control: shorthand for the Message.Control component

divider: shorthand for the Message.Divider component

Usage

API Proposal 3

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

codecov bot commented Sep 20, 2018 • edited

Codecov Report

kuzhelov commented Sep 20, 2018 • edited

mnajdova commented Sep 20, 2018

mnajdova commented Sep 20, 2018

alinais commented Sep 20, 2018

mnajdova commented Sep 20, 2018

mnajdova commented Sep 20, 2018

mnajdova commented Sep 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Sep 27, 2018

Choose a reason for hiding this comment

kuzhelov Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Sep 20, 2018 •

edited

codecov bot commented Sep 20, 2018 •

edited

kuzhelov commented Sep 20, 2018 •

edited

mnajdova commented Sep 20, 2018 •

edited

kuzhelov Oct 1, 2018 •

edited

kuzhelov Oct 1, 2018 •

edited

kuzhelov Oct 1, 2018 •

edited

mnajdova Oct 1, 2018 •

edited