-
Notifications
You must be signed in to change notification settings - Fork 55
feat(ChatItem): add attached prop, renamed gutterPosition to contentPosition #767
Changes from 5 commits
0f0aea4
28b7d88
26de235
e032213
6a9cd8b
39c6ebc
461e56a
687807f
d1bde1b
548e5e8
f7b1d99
3979105
a1b552e
a346bd8
fd6b621
aed43ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,9 +120,20 @@ class ChatMessage extends UIComponent<ReactProps<ChatMessageProps>, ChatMessageS | |
) : ( | ||
<> | ||
{!mine && | ||
Text.create(author, { defaultProps: { size: 'small', styles: styles.author } })} | ||
Text.create(author, { | ||
defaultProps: { | ||
size: 'small', | ||
styles: styles.author, | ||
className: `${ChatMessage.className}__author`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was needed for targeting this className in the chat item for displaying or not the author of the message. If we add this prop on the ChatMessage as well, we won't need this. |
||
}, | ||
})} | ||
{Text.create(timestamp, { | ||
defaultProps: { size: 'small', styles: styles.timestamp, timestamp: true }, | ||
defaultProps: { | ||
size: 'small', | ||
styles: styles.timestamp, | ||
timestamp: true, | ||
className: `${ChatMessage.className}__timestamp`, | ||
}, | ||
})} | ||
{Box.create(content, { defaultProps: { styles: styles.content } })} | ||
</> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,76 @@ | ||
import { ICSSInJSStyle, ComponentSlotStylesInput } from '../../../types' | ||
import { ChatItemVariables } from './chatItemVariables' | ||
import { ChatItemProps } from '../../../../components/Chat/ChatItem' | ||
import { pxToRem } from '../../../../lib' | ||
import ChatMessage from '../../../../components/Chat/ChatMessage' | ||
|
||
const chatMessageClassname = `& .${ChatMessage.className}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add these values to a constants file somewhere or export them from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
what about exporting a constants object from export const chatMessageMetadata = {
authorClassName: `${ChatMessage.className}__author`,
timestampClassname: `${ChatMessage.className}__timestamp`,
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this idea of adding metadata objects. Not sure whether we can add this to the UIComponent and use static field for it, as probably every components will have different structure for it, but exporting a constant for now may be enough... |
||
const chatMessageAuthorClassname = `& .${ChatMessage.className}__author` | ||
const chatMessageTimestampClassname = `& .${ChatMessage.className}__timestamp` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it's a selector, not class name 🐤 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, will rename those, although still not sure how to export this slot's classnames from the components... Should we add authorClassName, timestampClassName as static fields in the ChatMesage?! Maybe just a constants exported from the ChatMessage (although it will be not consistent as other class names defined). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first guess would be a static field, something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would like to avoid the name selectors as it indicates that we will have the class selector There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add a static field called cssSelectors, and will specify the selectors for the author and timestamp there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My opinion: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Later we can introduce a test that asserts that all keys that are presented in styles have matching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
`.${ChatItem.slotCssSelectors.author} + .${Another.slotCssSelectors.slot}`
|
||
|
||
const chatItemStyles: ComponentSlotStylesInput<ChatItemProps, ChatItemVariables> = { | ||
root: ({ props: p, variables: v }): ICSSInJSStyle => ({ | ||
position: 'relative', | ||
marginTop: v.margin, | ||
marginBottom: v.margin, | ||
}), | ||
root: ({ props: p, variables: v }): ICSSInJSStyle => { | ||
const { grouped } = p | ||
return { | ||
position: 'relative', | ||
...((!grouped || grouped === 'start') && { marginTop: pxToRem(16) }), | ||
mnajdova marked this conversation as resolved.
Show resolved
Hide resolved
|
||
...(grouped && | ||
grouped !== 'start' && { | ||
marginTop: pxToRem(2), | ||
[chatMessageAuthorClassname]: { | ||
display: 'none', | ||
}, | ||
[chatMessageTimestampClassname]: { | ||
display: 'none', | ||
}, | ||
}), | ||
marginBottom: 0, | ||
} | ||
}, | ||
|
||
gutter: ({ props: p, variables: v }): ICSSInJSStyle => ({ | ||
position: 'absolute', | ||
marginTop: v.gutterMargin, | ||
[p.gutterPosition === 'end' ? 'right' : 'left']: 0, | ||
}), | ||
gutter: ({ props: p, variables: v }): ICSSInJSStyle => { | ||
const { grouped } = p | ||
return { | ||
position: 'absolute', | ||
marginTop: v.gutterMargin, | ||
[p.gutterPosition === 'end' ? 'right' : 'left']: 0, | ||
...(grouped && | ||
grouped !== 'start' && { | ||
visibility: 'hidden', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have |
||
}), | ||
} | ||
}, | ||
|
||
message: ({ props: p, variables: v }): ICSSInJSStyle => ({ | ||
position: 'relative', | ||
marginLeft: v.messageMargin, | ||
marginRight: v.messageMargin, | ||
}), | ||
message: ({ props: p, variables: v }): ICSSInJSStyle => { | ||
const { grouped } = p | ||
return { | ||
mnajdova marked this conversation as resolved.
Show resolved
Hide resolved
|
||
position: 'relative', | ||
marginLeft: v.messageMargin, | ||
marginRight: v.messageMargin, | ||
...(grouped === 'middle' && { | ||
[chatMessageClassname]: { | ||
borderTopLeftRadius: 0, | ||
borderBottomLeftRadius: 0, | ||
paddingTop: pxToRem(5), | ||
paddingBottom: pxToRem(7), | ||
}, | ||
}), | ||
...(grouped === 'start' && { | ||
[chatMessageClassname]: { | ||
borderTopLeftRadius: pxToRem(3), | ||
borderBottomLeftRadius: 0, | ||
}, | ||
}), | ||
...(grouped === 'end' && { | ||
[chatMessageClassname]: { | ||
borderTopLeftRadius: 0, | ||
borderBottomLeftRadius: pxToRem(3), | ||
paddingTop: pxToRem(5), | ||
paddingBottom: pxToRem(7), | ||
}, | ||
}), | ||
} | ||
}, | ||
} | ||
|
||
export default chatItemStyles |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
import { ComponentSlotStylesInput, ICSSInJSStyle } from '../../../types' | ||
import { ChatMessageProps } from '../../../../components/Chat/ChatMessage' | ||
import { ChatMessageVariables } from './chatMessageVariables' | ||
import { pxToRem } from '../../../../lib' | ||
|
||
const chatMessageStyles: ComponentSlotStylesInput<ChatMessageProps, ChatMessageVariables> = { | ||
root: ({ props: p, variables: v }): ICSSInJSStyle => ({ | ||
display: 'inline-block', | ||
padding: v.padding, | ||
paddingLeft: v.padding, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mnajdova this is now causing a problem with Control Messages; the padding values are different there so they need to be updated via the fyi @jurokapsiar - Marija, Juraj has more input here and can show you the issue on Embed side There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was change because the paddingsTop and Bottom are different when the messages are grouped, while the left and right padding are always the same. I am not sure why embed are using the ChatMessage component for the Control messages, but sure let's take a look. |
||
paddingRight: v.padding, | ||
paddingTop: pxToRem(8), | ||
paddingBottom: pxToRem(10), | ||
borderRadius: v.borderRadius, | ||
border: v.border, | ||
color: v.color, | ||
|
@@ -23,7 +27,9 @@ const chatMessageStyles: ComponentSlotStylesInput<ChatMessageProps, ChatMessageV | |
}), | ||
|
||
author: ({ props: p, variables: v }): ICSSInJSStyle => ({ | ||
display: p.mine ? 'none' : undefined, | ||
...(p.mine && { | ||
display: 'none', | ||
}), | ||
marginRight: v.authorMargin, | ||
}), | ||
|
||
|
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 remember that we discussed this before, but can you please point me to results?
Why we decided to use
grouped
instead of SUI'sattached={true|'top'|'bottom'}
?https://react.semantic-ui.com/elements/segment/#variations-attached
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.
My bad for not linking the issue in the PR.. Here is the issue with the discussion around this: #588 We were considering using grouped, consecutive prop or additional component for representing the groups of messages. We decided there to use the grouped prop (which in the issue is only as an boolean, but it turned out that we must have some different words for describing the first, the last message and the middle messages in a batch. The options: start, end and middle were chosen because the start and end are the options we are using across all other components. Please share some additional proposals if you have 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.
start
andend
but that applies for horizontal direction.true
seems better thatmiddle
(especially if you consider the default value to befalse
)Chat.Item
grouped
prop for describing groups of chat items #588, we have not discussedattached
. As this term already exists in SUI and seems to be valid among several components, what's the reason for not using that?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.
Will add the attached prop
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.
speaking of the design language choices, there are apps that are using
grouped
term to express this concept (Telegram as an example).Thing that I don't like with attached is the following combination of prop name and
top
value:This is really hard to reason about what does this prop mean for the element, especially when it will occur in the following context:
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.
to me it seems that original version with
grouped
is something that reads better: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.
Let's go with
attached: boolean | 'top' | 'bottom'
for now, it satisfies the need for three different attached ChatItems which is sufficient for now, and in the future once we will add this prop in other component, it will be much easier for the users to learn to use it... We may consider changing this in the future if we receive complains for it. Can we agree with this?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.
yes, sure