-
Notifications
You must be signed in to change notification settings - Fork 55
feat(ChatItem): add attached prop, renamed gutterPosition to contentPosition #767
Conversation
src/components/Chat/ChatMessage.tsx
Outdated
defaultProps: { | ||
size: 'small', | ||
styles: styles.author, | ||
className: `${ChatMessage.className}__author`, |
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 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.
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.
really nice job, just a few comments to be addressed
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 comment
The 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 ChatMessage.tsx
so there's only one place we change them?
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.
Last question, how do we want to export the classNames for the slots? See the author and timestamp classNames in the ChatMessage component.
what about exporting a constants object from ChatMessage.tsx
?
export const chatMessageMetadata = {
authorClassName: `${ChatMessage.className}__author`,
timestampClassname: `${ChatMessage.className}__timestamp`,
}
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 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...
[p.gutterPosition === 'end' ? 'right' : 'left']: 0, | ||
...(grouped && | ||
grouped !== 'start' && { | ||
visibility: 'hidden', |
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 display: 'none'
on the author and timestamp and visibility: 'hidden'
here, any reason we cannot keep consistency? I'd expect display: 'none'
to work here as well
|
||
const chatMessageClassname = `& .${ChatMessage.className}` | ||
const chatMessageAuthorClassname = `& .${ChatMessage.className}__author` | ||
const chatMessageTimestampClassname = `& .${ChatMessage.className}__timestamp` |
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: it's a selector, not class name 🐤
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
My first guess would be a static field, something like ChatMessage.selectors.author
.
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.
Would like to avoid the name selectors as it indicates that we will have the class selector .
at the end of the string. We may add something like ChatMessage.classes.author, but if we decide in future to have some different selectors (other then the classes, then maybe your suggestion would make more sense for the user... Just like to mention that if we decide with the selectors, would like to specify the authors and timestamp there as .ui-chat__item__author
and .ui-chat__item__timestamp
respectively. @kuzhelov @Bugaa92 @layershifter what is your opinion on 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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion:
We call timestamp
and author
as slots. So may be it can be slotClassNames
field be defined as a static field on the matching 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.
Later we can introduce a test that asserts that all keys that are presented in styles have matching slotClassName
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.
Maybe slotCssSelectors
will be better name, as in future we may have more complex selectors that are not necessarily classNames. And defining the selector will allow the user to always use them consistently without adding the necessary selector for className for example.. What do you think?
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.
- As user I want to have CSS class names because I should be able to build custom selectors:
`.${ChatItem.slotCssSelectors.author} + .${Another.slotCssSelectors.slot}`
- We have
className
property on each component, it's strange to haveselectors
src/components/Chat/ChatItem.tsx
Outdated
@@ -17,6 +17,9 @@ import Box from '../Box/Box' | |||
import { ComponentSlotStylesPrepared } from '../../themes/types' | |||
|
|||
export interface ChatItemProps extends UIComponentProps, ChildrenComponentProps { | |||
/** Indicates whether the ChatItem is the part of a batch. */ | |||
grouped?: 'start' | 'middle' | 'end' |
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's attached={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.
- It's true that we use
start
andend
but that applies for horizontal direction. true
seems better thatmiddle
(especially if you consider the default value to befalse
)- In [RFC]
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:
<ChatItem attached='start' />
This is really hard to reason about what does this prop mean for the element, especially when it will occur in the following context:
...
<ChatItem attached />
<ChatItem attached='end' />
<ChatItem attached='start' /> // ???
<ChatItem />
...
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:
...
<ChatItem grouped='start' />
<ChatItem grouped />
<ChatItem grouped />
<ChatItem grouped='end' />
<ChatItem />
...
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
Guys I have one problem with adjusting the styles for the grouped messages when we are using the mine prop. Basically, if the ChatMessages have the mine prop, currently we are floating the message to the right and we are changing the color. The problem is, when the ChatItem is grouped and the ChatMessage is mine we should change the border radius of different corners. Looking into the API, maybe the Last question, how do we want to export the classNames for the slots? See the author and timestamp classNames in the ChatMessage component. |
-adjusted some styles according to the red-lines
# Conflicts: # src/components/Chat/ChatMessage.tsx # src/themes/teams/components/Chat/chatMessageStyles.ts
# Conflicts: # CHANGELOG.md
-added slotClassNames static prop to the ChatMessage
@@ -17,11 +17,14 @@ import Box from '../Box/Box' | |||
import { ComponentSlotStylesPrepared } from '../../themes/types' | |||
|
|||
export interface ChatItemProps extends UIComponentProps, ChildrenComponentProps { | |||
/** Attach ChatItem to other 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.
would suggest to omit component class names in the description - as it should just convey general purpose of the prop. Something like: Controls item's relation to other chat items
|
||
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 comment
The 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 padding
prop; the problem with this change is that padding
variable only changes the left and right paddings as we're now hardcoding top padding to 8 and bottom padding to 10
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 comment
The 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.
This PR adds the attached prop for the ChatItem, in order for the theme to be able to style differently the messages that are part of a batch. The value of the attached prop can be one of: b,oolean, 'top' or 'bottom'. The PR fixes #588
This is an example of this prop used with the teams theme: