-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Chat): positiong fixes for actions #2300
Conversation
Perf comparison
Generated by 🚫 dangerJS |
builder => builder.hover(selectors.message).snapshot('Hovers the first message'), | ||
builder => builder.click(selectors.message).snapshot('Focus the first message via mouse click'), | ||
(builder, keys) => | ||
builder.keys('body', keys.tab).snapshot('Focuses the first message via keyboard'), |
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.
nit: technically, this does not select the first message
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.
Good catch, renamed this step
packages/react/src/themes/teams/components/Attachment/attachmentStyles.ts
Outdated
Show resolved
Hide resolved
…stardust-ui/react into fix/chat-actions � Conflicts: � CHANGELOG.md
*/ | ||
unstable_pinned && { flip: { enabled: false } }, | ||
), | ||
[hasScrollableElement, position, offset, rtl, unstable_pinned, userModifiers], |
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 have userModifiers
as dependency there as they are an object, to avoid recreation of Popper
on each render I decided to use there deep memo strategy.
|
||
return <Ref innerRef={contentRef}>{React.Children.only(child) as React.ReactElement}</Ref> | ||
return <Ref innerRef={contentRef}>{React.Children.only(child)}</Ref> |
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.
Small type improvements there as in real world we allow to pass only React.ReactElement
(forced via React.Children.only()
)
const scrollParentElement = getScrollParent(contentRef.current) | ||
|
||
return scrollParentElement !== scrollParentElement.ownerDocument.body | ||
}, [contentRef]) |
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 don't think that using Ref
as useMemo
dependency works.
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.
As discussed in offline added a comment there.
* fix(Chat): positiong fixes for actions * add changelog entry * update screener steps * revert change, add comment (cherry picked from commit b7980fc)
Fixes #2299.