Chat: use resizeObserver to update scrollbar size and position #28111
Conversation
EugeniyKiyashko
force-pushed
the
24_2_chat_resizeObserver
branch
from
431ecff to
f54d98d
Compare
EugeniyKiyashko
force-pushed
the
24_2_chat_resizeObserver
branch
from
f54d98d to
351c3a1
Compare
EugeniyKiyashko
changed the title
packages/devextreme/testing/tests/DevExpress.ui.widgets/chatParts/messageList.tests.js
Show resolved
Hide resolved
EugeniyKiyashko
force-pushed
the
24_2_chat_resizeObserver
branch
from
b797f15 to
e07b912
Compare
EugeniyKiyashko
force-pushed
the
24_2_chat_resizeObserver
branch
from
e07b912 to
3bcb8f6
Compare
EugeniyKiyashko
force-pushed
the
24_2_chat_resizeObserver
branch
from
3bcb8f6 to
4b842e1
Compare
| @@ -50,7 +63,47 @@ class MessageList extends Widget<Properties> { | |||
|
|
|||
| this._renderMessageListContent(); | |||
|
|
|||
| this.update(); | |||
| this._attachResizeObserverSubscription(); | |||
There was a problem hiding this comment.
[SSR logic]
I believe it should be done in _renderContent, not _initMarkup.
Then u don't need if (hasWindow()) { inside of this method
| } | ||
|
|
||
| _isAttached(element: Element): boolean { | ||
| return !!contains(domAdapter.getBody(), element); |
There was a problem hiding this comment.
[copypaste, shadowDOM]
I think we should use isElementInDom from utils/dom instead of creating this new method.
Moreover, now the solution does now work in shadowDOM as far as I see
|
|
||
| if (this._suppressResizeHandling | ||
| && this._isAttached(target) | ||
| && isElementVisible(target as HTMLElement) |
There was a problem hiding this comment.
[refactoring]
Can we isolate these conditions? Let it be a first if and return if element is invisible or detached.
Now i don't like we do smth actions in this case + update cached height
| _resizeHandler({ contentRect, target }: ResizeObserverEntry): void { | ||
| const newHeight = contentRect.height; | ||
|
|
||
| if (this._suppressResizeHandling |
There was a problem hiding this comment.
[naming, ?excess cache?]
u name it _suppressResizeHandling, but i see u don't suppress, u scroll to last message.
So, what is this for, just for indicating it's a first call, right?
Then maybe we can get rid of this private cache and just do it this way?
const isAfterFirstRendering = this._containerClientHeight === 0; // or even undefined/null?There was a problem hiding this comment.
I can simplified it this way:
if (!isElementInDom(target) || !isElementVisible(target as HTMLElement)) {
return;
}
if (!isDefined(this._containerClientHeight)) {
this._scrollContentToLastMessage();
}
| }); | ||
| }); | ||
|
|
||
| QUnit.test('should be scrolled to the last message after being rendered inside an invisible element and display correctly when shown', function(assert) { |
There was a problem hiding this comment.
| QUnit.test('should be scrolled to the last message after being rendered inside an invisible element and display correctly when shown', function(assert) { | |
| QUnit.test('should be scrolled to the last message after showing if was initially rendered inside an invisible element', function(assert) { |
|
|
||
| assert.strictEqual(scrollTop !== 0, true); | ||
| assert.notEqual(scrollTop, 0); | ||
| assert.roughEqual(scrollTop, this.getScrollOffsetMax(), 1); |
There was a problem hiding this comment.
[minor]
please add assertion comments. It's not very informative now
| this._scrollable.scrollTo({ top: scrollOffsetTopMax }); | ||
| } | ||
|
|
||
| _getScrollContainer(): HTMLElement { |
There was a problem hiding this comment.
[refactoring]
do we really need it? Maybe subscribe just on root?
|
|
||
| this._suppressResizeHandling = false; | ||
| } else { | ||
| const heightChange = this._containerClientHeight - newHeight; |
There was a problem hiding this comment.
Could u please add a Unit test for this scenario? Smth like "scrollable should keep scrolling position after container resize if scrolling position is not bottom"
It was very difficult for me now to understand without unit tests what it's written for
|
|
||
| let { scrollTop } = target; | ||
|
|
||
| if (heightChange >= 1 || !isReachedBottom(target as HTMLDivElement, target.scrollTop, 0, 1)) { |
There was a problem hiding this comment.
[excess code?]
I don't understand why we need heightChange.
U're trying to say "we need some scroll if we are now on bottom and height is increased" but i see no difference with and without this code
|
|
||
| let { scrollTop } = target; | ||
|
|
||
| if (heightChange >= 1 || !isReachedBottom(target as HTMLDivElement, target.scrollTop, 0, 1)) { |
There was a problem hiding this comment.
| if (heightChange >= 1 || !isReachedBottom(target as HTMLDivElement, target.scrollTop, 0, 1)) { | |
| const isReachedBottom = isReachedBottom(target as HTMLDivElement, target.scrollTop, 0, 1); | |
| if (!isReachedBottom) { |
No description provided.