fix(ChatMessage): show timestamp if reactionGroup is provided #1100
Conversation
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
miroslavstastny
as code owners
mnajdova
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1100 +/- ##
==========================================
- Coverage 82.14% 82.09% -0.06%
==========================================
Files 716 717 +1
Lines 8554 8567 +13
Branches 1230 1233 +3
==========================================
+ Hits 7027 7033 +6
- Misses 1511 1518 +7
Partials 16 16
Continue to review full report at Codecov.
|
mnajdova
changed the title
| @@ -50,6 +50,9 @@ export interface ChatMessageProps | |||
| /** Menu with actions of the message. */ | |||
| actionMenu?: ShorthandValue | |||
|
|
|||
| /** Controls messages's relation to other chat messages. It is automatically set from the ChatItem */ | |||
There was a problem hiding this comment.
nit-nit: lets end description with dot, also, we might consider to use by instead of from, something like:
Controls messages's relation to other chat messages. Is automatically set by the ChatItem.
| }, | ||
| }) | ||
|
|
||
| // the box element is ChatMessage |
There was a problem hiding this comment.
comment may be a bit misleading - what we are testing here is not the Box element, but rather if messageElement is ChatMessage. Lets just omit box from the comment text
| }) | ||
|
|
||
| // the box element is ChatMessage | ||
| if (this.checkIfElementIsChatMessage(messageElement)) { |
There was a problem hiding this comment.
nit: we may shorten the name of predicate method:
if (this.isChatMessageElement(messageElement)) {
..
}-changed method name
| } | ||
|
|
||
| checkIfElementIsChatMessage = (element, prop?) => { | ||
| return _.get(element, `${prop ? `props.${prop}.` : ''}type.__isChatMessage`) |
There was a problem hiding this comment.
I would suggest to make this method's logic as simple as possible - then it would be much easier to reason of what this method is supposed to do at the place of usage:
// just single parameter
isChatMessageElement = (element) => ...As a next step, we might notice that this method relies on the aspects that are provided by another type, i.e. ChatMessage's __isChatMessage static prop, which name might change. Thus, I would suggest to put this check as part of the ChatMessage component's logic:
class ChatMessage extends ... {
static isChatMessageElement(element) => ...
}And, finally - while I do see the benefit of introducing second arg to original method to avoid duplication of prop fetching logic, we might just want to introduce a dedicated method for props fetching for that:
getElementProp = (element, propName) => { .. }once again, with preserving the benefit of reducing duplication, we would have yet additional one: this method is quite easy to reason about on the client's side.
As a result, we might end with something like:
if (ChatMessage.isTypeOfElement(messageElement) { .. }
if (ChatMessage.isTypeOfElement(getElementProp(messageElement, 'content')) { .. }
| return this.cloneElementWithCustomProps(messageElement, { attached }) | ||
| } | ||
|
|
||
| // the children element is ChatMessage |
There was a problem hiding this comment.
given that children, theoretically, may be a function, the following comment would be more correct:
the children is ChatMessage element
the same applies to the content
| {contentPosition === 'end' && gutterElement} | ||
| </> | ||
| ) | ||
| } | ||
|
|
||
| getMessageAugmentedWithChatMessageProps = styles => { |
There was a problem hiding this comment.
just to better deliver the main intent of this method - lets rename if to something like setAttachedPropValueForChatMessage
| // the content is ChatMessage | ||
| if (ChatMessage.isTypeOfElement(getElementProp(messageElement, 'content'))) { | ||
| return this.cloneElementWithCustomProps(messageElement, { attached }, 'content') | ||
| } |
There was a problem hiding this comment.
Can you please clarify this for me? Will we add attached prop only is element/children/content are ChatItem?
But, any HOC will break this: https://codesandbox.io/s/n4mm1ny55l 💣
There was a problem hiding this comment.
Yes, by default we will propagate the attached in these three use cases (which we see as the most common) - the attached prop can always be applied on the ChatMessage if it is wrapped... I am open for refactoring it if we have some better ideas.. (once the template mechanism will be available to the user, we can safely omit these changes)
There is update for the reactions + timestamp in Teams theme. When there are reaction on the message, the timestamp should always be shown.