Skip to content

Commit

Permalink
fix(): tall items collapse correctly close to bottom
Browse files Browse the repository at this point in the history
  • Loading branch information
petyosi committed Dec 25, 2021
1 parent b4b965d commit 9665bf2
Show file tree
Hide file tree
Showing 16 changed files with 306 additions and 143 deletions.
30 changes: 30 additions & 0 deletions e2e/collapsible-long-item.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
describe('list with scroll seek placeholders', () => {
beforeEach(async () => {
await page.goto('http://localhost:1234/collapsible-long-item')
await page.waitForSelector('#test-root div')
await page.waitForTimeout(100)
})

it('compensates correctly when collapsing an item', async () => {
await page.evaluate(() => {
const scroller = document.querySelector('#test-root > div')!
scroller.scrollBy({ top: -400 })
})

await page.waitForTimeout(200)

await page.evaluate(() => {
const button = document.querySelector('[data-index="90"] button') as HTMLButtonElement
button.click()
})

await page.waitForTimeout(200)

const scrollTop = await page.evaluate(() => {
const scroller = document.querySelector('#test-root > div')!
return scroller.scrollTop
})

expect(scrollTop).toBe(9200)
})
})
45 changes: 45 additions & 0 deletions e2e/collapsible-long-item.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import * as React from 'react'
import { Virtuoso } from '../src'

const Expanded = React.createContext([
false,
(_val: boolean) => {
void _val
},
] as const)

const Item = ({ index }: { index: number }) => {
const [expanded, setExpanded] = React.useContext(Expanded)

return (
<div style={{ border: '1px solid black', height: index === 90 && !expanded ? 600 : 100, display: 'flex', flexDirection: 'row' }}>
<div style={{ flex: 1 }}>Item {index}</div>
<button
onClick={() => {
setExpanded(!expanded)
}}
>
Toggle
</button>
</div>
)
}

const ExpandedProvider: React.FC = ({ children }) => {
const [expanded, setExpanded] = React.useState(false)
return <Expanded.Provider value={[expanded as any, setExpanded]}>{children}</Expanded.Provider>
}

export default function App() {
return (
<ExpandedProvider>
<Virtuoso
initialTopMostItemIndex={99}
followOutput={true}
totalCount={100}
itemContent={(index) => <Item index={index} />}
style={{ height: 600 }}
/>
</ExpandedProvider>
)
}
44 changes: 24 additions & 20 deletions e2e/toggle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,40 +17,44 @@ export default function App() {
},
[toggle, setToggle]
)

const toggleSize = React.useCallback(
(index: number) => {
setToggle((toggle) => ({ ...toggle, [index]: !toggle[index] }))
},
[setToggle]
)

return (
<div>
<button
onClick={() => {
setToggle((toggle) => ({ ...toggle, 88: !toggle[88] }))
setToggle((toggle) => ({ ...toggle, 99: !toggle[99] }))
toggleSize(99)
toggleSize(98)
}}
>
Add + shrink
</button>

setTimeout(() => {
setToggle((toggle) => ({ ...toggle, 99: !toggle[99] }))
setTimeout(() => {
setToggle((toggle) => ({ ...toggle, 88: !toggle[88] }))
setCount((count) => count + 1)
})
}, 500)
// setTimeout(() => {})
/*
setTimeout(() => {
setCount((count) => count + 1)
})*/
<button
onClick={() => {
toggleSize(99)
toggleSize(98)
toggleSize(90)
}}
>
Add + shrink
</button>

<button
onClick={() => {
// setToggle((toggle) => ({ ...toggle, 92: !toggle[92] }))
setCount((count) => count + 1)
setTimeout(() => {
setCount((count) => count + 1)
})
setCount((count) => count + 2)
toggleSize(90)
}}
>
Add two quickly
Add + shrink
</button>

<Virtuoso
computeItemKey={(key: number) => `item-${key.toString()}`}
totalCount={count}
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"name": "react-virtuoso",
"author": "Petyo Ivanov",
"sideEffects": false,
"version": "2.2.9-beta.1",
"version": "0.0.0-development",
"homepage": "https://virtuoso.dev/",
"license": "MIT",
"source": "src/index.tsx",
Expand Down
20 changes: 8 additions & 12 deletions src/List.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -169,14 +169,14 @@ export const Items = React.memo(function VirtuosoItems({ showTopList = false }:
const listState = useEmitterValue('listState')
const deviation = useEmitterValue('deviation')
const sizeRanges = usePublisher('sizeRanges')
const scrollHeightCallback = usePublisher('scrollHeight')
const scrollContainerStateCallback = usePublisher('scrollContainerState')
const itemContent = useEmitterValue('itemContent')
const groupContent = useEmitterValue('groupContent')
const trackItemSizes = useEmitterValue('trackItemSizes')
const itemSize = useEmitterValue('itemSize')
const log = useEmitterValue('log')

const ref = useChangedListContentsSizes(sizeRanges, itemSize, trackItemSizes, showTopList ? noop : scrollHeightCallback, log)
const ref = useChangedListContentsSizes(sizeRanges, itemSize, trackItemSizes, showTopList ? noop : scrollContainerStateCallback, log)
const EmptyPlaceholder = useEmitterValue('EmptyPlaceholder')
const ScrollSeekPlaceholder = useEmitterValue('ScrollSeekPlaceholder') || DefaultScrollSeekPlaceholder
const ListComponent = useEmitterValue('ListComponent')!
Expand Down Expand Up @@ -292,18 +292,16 @@ export interface Hooks {

export function buildScroller({ usePublisher, useEmitter, useEmitterValue }: Hooks) {
const Scroller: Components['Scroller'] = React.memo(function VirtuosoScroller({ style, children, ...props }) {
const scrollTopCallback = usePublisher('scrollTop')
const scrollHeightCallback = usePublisher('scrollHeight')
const scrollContainerStateCallback = usePublisher('scrollContainerState')
const ScrollerComponent = useEmitterValue('ScrollerComponent')!
const smoothScrollTargetReached = usePublisher('smoothScrollTargetReached')
const scrollerRefCallback = useEmitterValue('scrollerRef')

const { scrollerRef, scrollByCallback, scrollToCallback } = useScrollTop(
scrollTopCallback,
scrollContainerStateCallback,
smoothScrollTargetReached,
ScrollerComponent,
scrollerRefCallback,
scrollHeightCallback
scrollerRefCallback
)

useEmitter('scrollTo', scrollToCallback)
Expand All @@ -324,17 +322,15 @@ export function buildScroller({ usePublisher, useEmitter, useEmitterValue }: Hoo

export function buildWindowScroller({ usePublisher, useEmitter, useEmitterValue }: Hooks) {
const Scroller: Components['Scroller'] = React.memo(function VirtuosoWindowScroller({ style, children, ...props }) {
const scrollTopCallback = usePublisher('windowScrollTop')
const scrollHeightCallback = usePublisher('scrollHeight')
const scrollContainerStateCallback = usePublisher('scrollContainerState')
const ScrollerComponent = useEmitterValue('ScrollerComponent')!
const smoothScrollTargetReached = usePublisher('smoothScrollTargetReached')
const totalListHeight = useEmitterValue('totalListHeight')
const { scrollerRef, scrollByCallback, scrollToCallback } = useScrollTop(
scrollTopCallback,
scrollContainerStateCallback,
smoothScrollTargetReached,
ScrollerComponent,
noop,
scrollHeightCallback
noop
)

useIsomorphicLayoutEffect(() => {
Expand Down
18 changes: 18 additions & 0 deletions src/domIOSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as u from '@virtuoso.dev/urx'

export const domIOSystem = u.system(
() => {
const scrollContainerState = u.stream<[number, number]>()
const scrollTop = u.stream<number>()
const deviation = u.statefulStream(0)
const smoothScrollTargetReached = u.stream<true>()
Expand All @@ -14,10 +15,27 @@ export const domIOSystem = u.system(
const scrollBy = u.stream<ScrollToOptions>()
const scrollingInProgress = u.statefulStream(false)

u.connect(
u.pipe(
scrollContainerState,
u.map(([scrollTop]) => scrollTop)
),
scrollTop
)

u.connect(
u.pipe(
scrollContainerState,
u.map(([, scrollHeight]) => scrollHeight)
),
scrollHeight
)

u.connect(scrollTop, statefulScrollTop)

return {
// input
scrollContainerState,
scrollTop,
viewportHeight,
headerHeight,
Expand Down
2 changes: 1 addition & 1 deletion src/followOutputSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export const followOutputSystem = u.system(
([, followOutput]) => {
const cancel = u.handleNext(atBottomState, (state) => {
if (followOutput && !state.atBottom && state.notAtBottomBecause === 'SIZE_INCREASED' && !pendingScrollHandle) {
u.getValue(log)('scrolling to bottom due to increased size', LogLevel.DEBUG)
u.getValue(log)('scrolling to bottom due to increased size', {}, LogLevel.DEBUG)
scrollToBottom('auto')
}
})
Expand Down
3 changes: 2 additions & 1 deletion src/gridSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function buildItems(startIndex: number, endIndex: number) {
export const gridSystem = u.system(
([
{ overscan, visibleRange, listBoundary },
{ scrollTop, viewportHeight, scrollBy, scrollTo, smoothScrollTargetReached },
{ scrollTop, viewportHeight, scrollBy, scrollTo, smoothScrollTargetReached, scrollContainerState },
stateFlags,
scrollSeek,
{ propsReady, didMount },
Expand Down Expand Up @@ -258,6 +258,7 @@ export const gridSystem = u.system(
windowScrollTo,
useWindowScroll,
windowScrollTop,
scrollContainerState,
initialItemCount,
...scrollSeek,

Expand Down
6 changes: 3 additions & 3 deletions src/hooks/__mocks__/useScrollTop.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
type CallbackRefParam = HTMLElement | null

export default function useSize(callback: (scrollTop: number) => void) {
export default function useSize(callback: (state: [number, number]) => void) {
const scrollerRef = (elRef: CallbackRefParam) => {
if (elRef) {
;(elRef as any).triggerScroll = (scrollTop: number) => {
callback(scrollTop)
;(elRef as any).triggerScroll = (state: [number, number]) => {
callback(state)
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/hooks/useChangedChildSizes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ export default function useChangedListContentsSizes(
callback: (ranges: SizeRange[]) => void,
itemSize: SizeFunction,
enabled: boolean,
scrollHeightCallback: (height: number) => void,
scrollContainerStateCallback: (state: [number, number]) => void,
log: Log
) {
return useSize((el: HTMLElement) => {
const ranges = getChangedChildSizes(el.children, itemSize, 'offsetHeight', log)
const scrollableElement = el.parentElement!.parentElement!
scrollHeightCallback(scrollableElement.scrollHeight)
scrollContainerStateCallback([Math.max(scrollableElement.scrollTop, 0), scrollableElement.scrollHeight])
if (ranges !== null) {
callback(ranges)
}
Expand Down
14 changes: 7 additions & 7 deletions src/hooks/useScrollTop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ function approximatelyEqual(num1: number, num2: number) {
}

export default function useScrollTop(
scrollTopCallback: (scrollTop: number) => void,
scrollContainerStateCallback: (state: [number, number]) => void,
smoothScrollTargetReached: (yes: true) => void,
scrollerElement: any,
scrollerRefCallback: (ref: ScrollerRef) => void = u.noop,
scrollHeightCallback: (height: number) => void = u.noop
scrollerRefCallback: (ref: ScrollerRef) => void = u.noop
) {
const scrollerRef = useRef<HTMLElement | null | Window>(null)
const scrollTopTarget = useRef<any>(null)
Expand All @@ -26,8 +25,7 @@ export default function useScrollTop(
(el as any) === window || (el as any) === document ? window.pageYOffset || document.documentElement.scrollTop : el.scrollTop
const scrollHeight = (el as any) === window ? document.documentElement.scrollHeight : el.scrollHeight

scrollHeightCallback(scrollHeight)
scrollTopCallback(Math.max(scrollTop, 0))
scrollContainerStateCallback([Math.max(scrollTop, 0), scrollHeight])

if (scrollTopTarget.current !== null) {
if (scrollTop === scrollTopTarget.current || scrollTop <= 0 || scrollTop === el.scrollHeight - correctItemSize(el, 'height')) {
Expand All @@ -40,7 +38,7 @@ export default function useScrollTop(
}
}
},
[scrollTopCallback, smoothScrollTargetReached, scrollHeightCallback]
[scrollContainerStateCallback, smoothScrollTargetReached]
)

useEffect(() => {
Expand Down Expand Up @@ -86,7 +84,7 @@ export default function useScrollTop(
// with the scrollTop
// scroller is already at this location
if (approximatelyEqual(offsetHeight, scrollHeight) || location.top === scrollTop) {
scrollTopCallback(scrollTop)
scrollContainerStateCallback([scrollTop, scrollHeight])
if (isSmooth) {
smoothScrollTargetReached(true)
}
Expand All @@ -108,11 +106,13 @@ export default function useScrollTop(
scrollTopTarget.current = null
}

console.log('scrolling to', location)
scrollerElement.scrollTo(location)
}

function scrollByCallback(location: ScrollToOptions) {
if (scrollTopTarget.current === null) {
console.log('scrolling by', location)
scrollerRef.current!.scrollBy(location)
}
}
Expand Down
Loading

2 comments on commit 9665bf2

@Ethorsen
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @petyosi ,

You forgot a few console.log in this PR. :)

Thx for the continuous hard work. Happy Christmas to you

@petyosi
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ethorsen thanks for spotting that, pushed a fix. Happy holidays to you too.

Please sign in to comment.