fix(ChatMessage): actions menu's appearance #1739
Conversation
kuzhelov
requested review from
dvdzkwsk,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
| @@ -153,14 +157,16 @@ class ChatMessage extends UIComponent<WithAsProp<ChatMessageProps>, ChatMessageS | |||
| }, | |||
|
|
|||
| focus: event => { | |||
| if (this.messageRef) { | |||
There was a problem hiding this comment.
this check was guaranteed to pass
| state = { | ||
| focused: false, | ||
| isFromKeyboard: false, | ||
| messageDomNode: null, |
There was a problem hiding this comment.
it is necessary to use state to store this DOM node, as otherwise Popper component that is used for actionsMenu rendering won't update its target once messageDomNode is set
| return ( | ||
| <Popper targetRef={this.state.messageDomNode} position="above" align="end"> | ||
| {({ scheduleUpdate }) => { | ||
| this.updateActionsMenuPosition = scheduleUpdate |
There was a problem hiding this comment.
it is necessary to trigger position updates for the cases when actions menu appear, as it has width of 0 when is hidden (https://github.com/stardust-ui/react/blob/master/packages/react/src/themes/teams/components/Chat/chatMessageStyles.ts#L95)
| <ElementType | ||
| onBlur={this.handleBlur} | ||
| onFocus={this.handleFocus} | ||
| onMouseEnter={() => this.updateActionsMenuPosition()} |
There was a problem hiding this comment.
is it ok not to update the position when you are only using keyboard?
There was a problem hiding this comment.
yes, as logic of this.handleFocus is responsible for making position updates in this case: https://github.com/stardust-ui/react/pull/1739/files#diff-36ca6d6c5aa2d1fa2b0e2d9f04902716R168
Codecov Report
@@ Coverage Diff @@
## master #1739 +/- ##
==========================================
+ Coverage 71.06% 71.14% +0.08%
==========================================
Files 861 857 -4
Lines 7171 7115 -56
Branches 2082 2054 -28
==========================================
- Hits 5096 5062 -34
+ Misses 2069 2047 -22
Partials 6 6
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1739 +/- ##
==========================================
- Coverage 69.99% 69.98% -0.02%
==========================================
Files 868 868
Lines 7323 7336 +13
Branches 2117 2119 +2
==========================================
+ Hits 5126 5134 +8
- Misses 2191 2196 +5
Partials 6 6
Continue to review full report at Codecov.
|
kuzhelov
changed the title
…ardust-ui/react into fix/chat-message-actions-menu
…ardust-ui/react into fix/chat-message-actions-menu
This PR changes the way positioning is applied to
actionsMenuofChatMessage- nowPoppercomponent is used to control positioning of this floating section.This change is necessary to handle situations where
actionsMenu's content may be cut by the edges of the viewport - this may be the case for messages with short content and long list ofactionMenu's items.Before
Actions menu content is cut off.
Now
Actions menu's position is adjusted.
Notes on the approach
To handle
actionsMenucut-off case, the strategy of usingPopper's positioning was taken. Essentially, previously the set of static CSS props (in a sense that they were prescribed once and for entire component's time) were used for absolutely positionactionsMenu, and these CSS values were part of the theme.Unfortunately, these values should be set dynamically, in order to handle the cases when viewport's width is not enough to show the entire
actionMenu's content - that's whyPopper's positioning utilities were considered to be part of the solutionThere were an alternative strategy to use
Popperwrapper for the slot's content, instead of providingPopperas part ofChatMessage's implementation. However, this introduces couple of problems:actionMenupositioning: one is the slot's wrapper, and another one - CSS values defined in themeThus, it was decided to pay the costs of extensibility and introduce
Popperpositioning foractionsMenuas part ofChatMessage's implementation, at least for now.