Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(useInfiniteScroll): improve visibility check #3212

Merged
merged 6 commits into from Jul 30, 2023
Merged

fix(useInfiniteScroll): improve visibility check #3212

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0181645
fix(useInfiniteScroll): adjust element offsetParent check
erikkkwu Jul 5, 2023
4328b62
Merge branch 'vueuse:main' into main
erikkkwu Jul 16, 2023
4d996d9
fix(useInfiniteScroll): use useElementVisibility instead of offsetPar…
erikkkwu Jul 16, 2023
f09d212
chore(useInfiniteScroll): format code
erikkkwu Jul 16, 2023
d9eeb3f
refactor(useInfiniteScroll): rename and destructuring element properties
erikkkwu Jul 16, 2023
f33749e
refactor(useInfiniteScroll): making tests more realistic
erikkkwu Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/useInfiniteScroll/index.ts
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ export function useInfiniteScroll(
state.measure()

const el = toValue(element) as HTMLElement
if (!el || !el.offsetParent)
Copy link
Contributor

@genu genu Jul 10, 2023 •
edited

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 a document instance.

We need to check if el is a instance of HTMLDocument first, instead of assuming its an HTMLElement

Copy link
Contributor Author

@erikkkwu erikkkwu Jul 10, 2023 •
edited

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?

Copy link
Contributor

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 an HTMLElement:

const el = toValue(element) as HTMLElement
    if (!el || !el.offsetParent)
      return

This is not correct because the function definition itself tells us that element could take 4 different shapes:

export function useInfiniteScroll(
  element: MaybeRefOrGetter<HTMLElement | SVGElement | Window | Document | null | undefined>,
...
)

coincidentally only HTMLElemewnt has an offsetParent

Pinging @antfu for feedback.

Copy link
Member

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)?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

if (!el || el.offsetParent === null)
return

const isNarrower = (direction === 'bottom' || direction === 'top')
Expand Down