New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(useInfiniteScroll): improve visibility check #3212
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0181645
fix(useInfiniteScroll): adjust element offsetParent check
erikkkwu 4328b62
Merge branch 'vueuse:main' into main
erikkkwu 4d996d9
fix(useInfiniteScroll): use useElementVisibility instead of offsetPar…
erikkkwu f09d212
chore(useInfiniteScroll): format code
erikkkwu d9eeb3f
refactor(useInfiniteScroll): rename and destructuring element properties
erikkkwu f33749e
refactor(useInfiniteScroll): making tests more realistic
erikkkwu File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
import { flushPromises } from '@vue/test-utils' | ||
import { describe, expect, it, vi } from 'vitest' | ||
import { ref } from 'vue-demi' | ||
import { useElementVisibility } from '../useElementVisibility' | ||
import { useInfiniteScroll } from '.' | ||
|
||
vi.mock('../useElementVisibility') | ||
describe('useInfiniteScroll', () => { | ||
it('should be defined', () => { | ||
expect(useInfiniteScroll).toBeDefined() | ||
}) | ||
|
||
it.each([ | ||
[ref(givenMockElement())], | ||
[givenMockElement()], | ||
[document], | ||
[window], | ||
])('should calls the loadMore handler, when element is visible', (target) => { | ||
const mockHandler = vi.fn() | ||
givenElementVisibilityRefMock(true) | ||
|
||
useInfiniteScroll(target, mockHandler) | ||
|
||
expect(mockHandler).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
it('should calls the loadMore handler, when element visibility state form hidden to visible', async () => { | ||
const mockHandler = vi.fn() | ||
const mockElement = givenMockElement() | ||
const visibilityRefMock = givenElementVisibilityRefMock(false) | ||
|
||
useInfiniteScroll(mockElement, mockHandler) | ||
|
||
expect(mockHandler).not.toHaveBeenCalled() | ||
|
||
visibilityRefMock.value = true | ||
await flushPromises() | ||
|
||
expect(mockHandler).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
it('should call the loadMore handler, when user scrolls', async () => { | ||
const mockElementScrollHeight = 100 | ||
const mockHandler = vi.fn() | ||
const mockElement = givenMockElement({ | ||
scrollHeight: mockElementScrollHeight, | ||
}) | ||
givenElementVisibilityRefMock(true) | ||
|
||
useInfiniteScroll(mockElement, mockHandler) | ||
mockElement.scrollTop = mockElementScrollHeight | ||
mockElement.dispatchEvent(new Event('scroll')) | ||
await flushPromises() | ||
|
||
expect(mockHandler).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
function givenMockElement({ | ||
scrollHeight = 0, | ||
} = {}): HTMLDivElement { | ||
const mockElement = document.createElement('div') | ||
Object.defineProperty(mockElement, 'scrollHeight', { | ||
value: scrollHeight, | ||
}) | ||
return mockElement | ||
} | ||
|
||
function givenElementVisibilityRefMock(defaultValue: boolean) { | ||
const mockVisibilityRef = ref(defaultValue) | ||
vi.mocked(useElementVisibility).mockReturnValue(mockVisibilityRef) | ||
return mockVisibilityRef | ||
} | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This fails when
el
is adocument
instance.We need to check if
el
is a instance ofHTMLDocument
first, instead of assuming its anHTMLElement
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 discovered that although the approach of determining the element offsetParent solves the issue of an infinite loop when v-show is false #3143, it also results in the load more functionality not working when v-show is switched to true. The solution I came up with is to use MutationObserver to listen for changes and trigger the load more. do you have any better suggestions?
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 need to review the code more, but I think the issue in #3143 needs to be solved in a different way.
The changes that were made by @antfu coerces the
element
to be anHTMLElement
:This is not correct because the function definition itself tells us that
element
could take 4 different shapes:coincidentally only
HTMLElemewnt
has anoffsetParent
Pinging @antfu for feedback.
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 am not very familiar with this, but I guess we could do
!el || (('offsetParent' in el) && !el.offsetParent)
?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 can, but it will have another problem that load more callback will not trigger when the v-show from false to true
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.
My recommendation is to re-open that issue and revert the fix that was made because It had caused a regression. Then we can move the discussion on how to fix it there or another PR.
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.
@genu Any update on this? :)
If I understand correctly, you just need a PR that reverts the offsetParent check?
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.
This PR was updated and I reviewed again. In my testing, everything looks good now.