-
Notifications
You must be signed in to change notification settings - Fork 55
feat(chat message): add author
and timestamp
props
#242
Conversation
ba64493
to
aa4ac4f
Compare
CHANGELOG.md
Outdated
@@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
### Features | |||
- Add `FocusZone` to `renderComponent`, change `Menu` behavior to support arrow keys @tomasiser ([#116](https://github.com/stardust-ui/react/pull/116)) | |||
- Add `value`, `disabled`, `checked`, `defaultChecked` and `onChange` props to `Radio` component @mnajdova ([#206](https://github.com/stardust-ui/react/pull/206)) | |||
- Add `author` and `timestamp` props for `Chat.Message` component @Bugaa92 ([#242](https://github.com/stardust-ui/react/pull/242)) |
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.
consider to move this entry to 'Unreleased' section
src/components/Chat/ChatMessage.tsx
Outdated
timestamp={timestamp} | ||
content={content} | ||
size="sm" | ||
styles={{ root: { marginRight: pxToRem(10) } }} |
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 shouldn't hardcode styles in component's implementation code. Lets rather introduce timestamp
part in the ChatMessage's styles, and provide this margin there
src/components/Chat/ChatMessage.tsx
Outdated
|
||
private renderText = (content: string, timestamp?: boolean) => ( | ||
<Text | ||
timestamp={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.
lets use two separate components for that - one for displaying timestamp
, another one - to display content
. This is necessary because Text
component is seen to be a basic one, so it would support either timestamp
variant, or regular content
one. More than that, it is highly likely that timestamp
variant won't be introduced as part of core Stardust
@@ -3,6 +3,7 @@ import { IChatMessageProps } from '../../../../components/Chat/ChatMessage' | |||
import { IChatMessageVariables } from './chatMessageVariables' | |||
import { pxToRem } from '../../../../lib' | |||
|
|||
const rem10 = pxToRem(10) |
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 rather suggest name like px10asRem
- otherwise the name is a bit misleading and reads like '10 rem'
@@ -0,0 +1,7 @@ | |||
export interface IChatVariables { | |||
backgroundColor: string |
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 better to be handled by the code of examples for now - more than that, we could adjust padding aspects there as well
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.
Actually, I added the backgroundColor and the padding to the variables with some defaults that would match the Team's theme, as we are developing this theme, so why not have this look by default. Please share your thoughts on this. Also, I added avatar slot in the ChatMessage, for configuring the statusBorderColor to match the background. Actually I have one thing that I am not 100% sure that should be implement this way. The Chat component has it's own backgroundColor and this color should match the Avatar's statusBorderColor. The problem is that, the Avatar is inside the ChatMessage component, and I am not confident that the Chat component should define the avatar's slot in the ChatMessage component. For now, those colors are independent variables in the Chat, and the ChatMessage's avatar slot component. What do you think about this @kuzhelov? This is how the default look of the examples looks now:
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.
sounds quite reasonable, thanks!
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.
agreed to not introduce these two as variables for now, just inline them into styles directly
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 are going to stick with the backgroundColor as variable in order to reuse the siteVariables. Paddings are deleted from variables
src/components/Chat/ChatMessage.tsx
Outdated
avatar?: ItemShorthand | ||
children?: ReactChildren | ||
className?: string | ||
content?: any | ||
mine?: boolean | ||
styles?: IComponentPartStylesInput | ||
timestamp?: string | ||
variables?: ComponentVariablesInput |
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.
Why don't we make the text and the author shorthands for the Text 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.
yeah - have discussed with @mnajdova, here proposal is essentially to introduce author
and timestamp
shorthands (slots) for the ChatMessage
component, and use Text
as a default component for these.
Totally support this idea 👍
Can we close #41 in favor of this one? |
sure, lets do that |
Codecov Report
@@ Coverage Diff @@
## master #242 +/- ##
==========================================
+ Coverage 91.84% 92.15% +0.31%
==========================================
Files 61 61
Lines 1042 1058 +16
Branches 154 155 +1
==========================================
+ Hits 957 975 +18
+ Misses 82 80 -2
Partials 3 3
Continue to review full report at Codecov.
|
CHANGELOG.md
Outdated
@@ -17,6 +17,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
|
|||
## [Unreleased] | |||
|
|||
### BREAKING CHANGES | |||
- Fixed `Divider` wrong usage of the `typeSecondary{color, backgroundColor}` and `default{color, backgroundColor}` variables; renamed `default{color, backgroundColor}` variables to `color` and `backgroundColor` @mnajdova ([#234](https://github.com/stardust-ui/react/pull/234)) |
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.
👍
|
||
export interface IChatMessageProps { | ||
as?: any | ||
author?: 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.
👍
# Conflicts: # CHANGELOG.md # src/components/Chat/ChatMessage.tsx
Chat.Message:
author
andtimestamp
This PR introduces
author
andtimestamp
props forChat.Message
subcomponentTODO
author
Rendered only in "their" messages (
mine=false
).renders
timestamp
renders