feat(Reaction): add Reaction component and use it in the ChatMessage #959
Conversation
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
miroslavstastny
as code owners
Codecov Report
@@ Coverage Diff @@
## master #959 +/- ##
========================================
+ Coverage 81% 81.1% +0.1%
========================================
Files 665 671 +6
Lines 8528 8612 +84
Branches 1443 1514 +71
========================================
+ Hits 6908 6985 +77
- Misses 1606 1612 +6
- Partials 14 15 +1
Continue to review full report at Codecov.
|
-fixed screener diffs
-fixing error on Reaction example
| @@ -22,6 +23,9 @@ const getChatMessageEvaluatedStyles = (p: ChatItemProps) => ({ | |||
| paddingTop: pxToRem(5), | |||
| paddingBottom: pxToRem(7), | |||
| ...getPositionStyles(p), | |||
| [chatMessageContentClassNameSelector]: { | |||
There was a problem hiding this comment.
I had to conditionally change the display prop on the content, because if the author and timestamp are not visible in Teams theme, the reactions should appear after the content. No better idea in the moment (tried added float elements, but then the paddings are broken...) I didn't want to create additional element for wrapping this, so this seemed like the only option.
mnajdova
changed the title
packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts
Outdated
Show resolved
Hide resolved
-fixing imports
There was a problem hiding this comment.
minor comments, everything else looks like @layershifter covered it, nice job 👍
docs/src/examples/components/Reaction/Types/ReactionExample.shorthand.tsx
Outdated
Show resolved
Hide resolved
-addressed comments on PR
docs/src/examples/components/Reaction/Types/ReactionGroupExample.shorthand.tsx
Outdated
Show resolved
Hide resolved
-added description for the reactionGroupPosition prop
-added new components in the theme typings
# Conflicts: # CHANGELOG.md
docs/src/examples/components/Chat/Types/ChatMessageExampleStyled.shorthand.tsx
Outdated
Show resolved
Hide resolved
| @@ -104,12 +112,15 @@ class ChatMessage extends UIComponent<ReactProps<ChatMessageProps>, ChatMessageS | |||
| timestamp: customPropTypes.itemShorthand, | |||
| onBlur: PropTypes.func, | |||
| onFocus: PropTypes.func, | |||
| reactionGroup: customPropTypes.itemShorthand, | |||
There was a problem hiding this comment.
client's voice in me would say that originally taken reactions name would work better - although we might want to be absolutely pedantic in terms of matching prop names with component types used as a default value, I would rather expect that these two concepts will be decoupled from each other, in general. These are the main reasons for that:
reactionsprop name looks nicer to the client- in general we might foresee that, for some reason,
ReactionsGroupname that was originally taken for this component will change. Then, if we would still strive for strict correspondence between these two, we would need to change the name of the prop in the API as well - and, actually, it might be not the only place where this change would be necessary
There was a problem hiding this comment.
Yep, the reason I renamed it is correlated with the next comment you've added...
- I would have
reactions={{items: [....]}}which looks even more awkward.. With preserving the namereactionGroup, I would feel less bad for having items there... - At this moment, it seems more consistent with the other
shorthands.
These are the only reasions why I have chosen this name. I may change it, but then we will have on the same component the props: actionMenu and reactions (inconsistency even in the same component's API). Let me know what do you think...
Additional thought, maybe for some of the components that represent group of other components, like the Menu, ButtonGroup, ReactionGroup etc, we may create special shorthand that will be list and will be applied on the collection shorthand that they represent. This would solve lots of these inconsistency and awkward api...
There was a problem hiding this comment.
yes, absolutely agree - lets just keep this in mind and defer to follow-up PR the necessary changes for shorthand to be able to process array values. This would solve this problem, as well as the problems that @layershifter has reminded us about here: https://github.com/stardust-ui/react/pull/959/files#r260743449
There was a problem hiding this comment.
Here is the created issue for reference: #984
docs/src/examples/components/Chat/Content/ChatExampleReactionGroup.shorthand.tsx
Show resolved
Hide resolved
The aim of this PR is to implement the RFC proposed here: #942
The PR adds the following things:
Reactioncomponent, which containsiconandcount(maybe this should be shorthand of theButtoncomponent?)reactionscollection shorthand in theChatMessage.reactionsPositionprop in theChatMessagewith the values'start'and'end', to indicate whether they should appear before or after the content in the generated HTML structure.Chat message with reactions:
