-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Chat): add actionMenu
prop for ChatMessage
#811
Conversation
…at/chat-message-actions
actionMenu
prop for ChatMessage
actionMenu
prop for ChatMessage
actionMenu
prop for ChatMessage
actionMenu
prop for ChatMessage
…thub.com/stardust-ui/react into feat/chat-message-actions # Conflicts: # CHANGELOG.md
…thub.com/stardust-ui/react into feat/chat-message-actions # Conflicts: # CHANGELOG.md # packages/react/src/components/Chat/ChatMessage.tsx
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
=========================================
Coverage ? 93.54%
=========================================
Files ? 21
Lines ? 728
Branches ? 69
=========================================
Hits ? 681
Misses ? 47
Partials ? 0 Continue to review full report at Codecov.
|
@@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
|
|||
### Features | |||
- Accessibility for menu divider @jurokapsiar ([#822](https://github.com/stardust-ui/react/pull/822)) | |||
- Add `actionMenu` prop to `ChatMessage` component @layershifter ([#811](https://github.com/stardust-ui/react/pull/811)) |
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.
just putting my vote on that: would rather prefer actions
than actionsMenu
for the following reasons
- it will be easier to avoid breaking changes in future - for example, if at some point it will become an
actionsPanel
- it is consistent with the naming taken for
Attachment
component, where there is anaction
slot
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.
but given the example from description, agree that it is definitely better to use singular menu
noun in the name of the prop, to properly reflect underlying semantics
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, a bit surprised that we are not handling array as 'primitive' shorthand values - is there any particular reason for that? Might expect that this feature is necessary for all the places where collection-container component stays as a default one for shorthand value
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 can be wrong, but this decision came from the consistency point: you can pass only ShorthandValue
or ShorthandValue[]
. However, I understand your point, had the same feeling about it.
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.
got it, thank you @layershifter! actually, this original problem (actions
and necessity to compromise on actionsMenu
) is a good example that might serve as an inspiration for us to further refine this moment in future
packages/react/src/themes/teams/components/Chat/chatMessageVariables.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts
Outdated
Show resolved
Hide resolved
…thub.com/stardust-ui/react into feat/chat-message-actions # Conflicts: # CHANGELOG.md # packages/react/src/components/Chat/ChatMessage.tsx # packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts
event.preventDefault() | ||
}, | ||
handleBlur = (e: React.FocusEvent) => { | ||
const shouldPreserveFocusState = _.invoke(e, 'currentTarget.contains', e.relatedTarget) |
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.
could you, please, suggest the case we would like to cover with 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.
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.
then, frankly, I am a bit confused with onBlur
event being raised unconditionally - so, for instance, even if shouldPreserveFocusState
is true
(and, as a consequence, even this.state.focused
remains to be true) - even in this situation we will trigger blur
event for ChatMessage
.
So, couple of things that I would like to solve here:
- ensure that I am not missing anything in this thought
- and, in case if so, it seems that we need to resolve this emerged problem that is about implicit reliance of events subscriptions - we should strive for component's behavioural intents being explicit
Could you, please, to resolve this issue for now, just provide a comment that will describe the intent of these lines? Thank you!
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.
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, totally agree about naming - this should help. However, it seems that many things are involved here, so it is, probably, much better to address naming and all the related moments in a dedicated PR - what do you think?
@@ -56,6 +62,13 @@ export interface ChatMessageProps | |||
/** A message can format the badge to appear at the start or the end of the message. */ | |||
badgePosition?: 'start' | 'end' | |||
|
|||
/** | |||
* Called after user's focus. |
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 need to update this one :)
…thub.com/stardust-ui/react into feat/chat-message-actions
actions
vsactionMenu
Initially, I was thinking about
actions
, but this shorthand points toMenu
component and it was really confusing why the code below will be broken:Also picks changes from #765.