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.

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

Observe element for size changes with ResizeObserver #405

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

piecyk
Copy link
Collaborator

@piecyk piecyk commented Oct 13, 2022

Was looking on our options how to add observing element for size changes using ResizeObserver fix #376. What if we would change a bit the api, end expose measureElement on top level, not on the item. This would allow use to memo few of them for example in grid and fix #391

Then we nee to bound measureElement with index, we can do it via attribute by adding data-index={virtualRow.index}.

@piecyk piecyk force-pushed the feat/observe branch 4 times, most recently from 109fc06 to b50ee10 Compare October 14, 2022 16:10
@piecyk piecyk requested a review from tannerlinsley October 14, 2022 16:10
@piecyk piecyk force-pushed the feat/observe branch 5 times, most recently from 9c444d4 to d99d6e6 Compare October 16, 2022 18:14
@piecyk piecyk force-pushed the feat/observe branch 3 times, most recently from 6ec5b8d to 7cbc5cd Compare October 24, 2022 07:04
@piecyk
Copy link
Collaborator Author

piecyk commented Oct 24, 2022

As note, for example having two lists and keeping only one visible at the time, other hidden by display none. In this context we would like to skip updating sizes for the hidden one as ResizeObserver will be called on display change.

One solution could be, if virtualizer would expose measurementsCache and indexFromElement like in this PR

export const useVirtualizer2 = <TScrollElement extends Element, TItemElement extends HTMLElement>({
  updateSize = true,
  ...options
}: VirtualizerOptions<TScrollElement, TItemElement>) => {
  const virtualizer = useVirtualizer({
    ...options,
    measureElement: (element: TItemElement, instance: Virtualizer<TScrollElement, TItemElement>) => {
      const getSize = () => measureElement(element, instance)

      if (updateSize) {
        return getSize()
      } else {
        const item = instance.measurementsCache[instance.indexFromElement(element)]

        return item ? item.size : getSize()
      }
    },
  })

  return virtualizer
}

@piecyk piecyk merged commit d257898 into TanStack:beta Nov 7, 2022
@piecyk piecyk deleted the feat/observe branch November 7, 2022 20:12
@andylwelch
Copy link

Thank you, this is amazing! I can delete so much logic to trigger resize manually

ivan-aksamentov added a commit to neherlab/pan-genome-visualization that referenced this pull request Nov 20, 2022
This was due to a bug in react-virtual:
TanStack/virtual#391
TanStack/virtual#405

Upgrade to v3 solves the issue
@lukesmurray
Copy link

lukesmurray commented Dec 8, 2022

@piecyk this change is nice overall, but makes a specific use case a bit more complicated.

Previously we could ignore the API of measureElement and provide our own. For instance measureElement: (height) => {...}. Then instead of assigning the measureElement function to the ref when we render row items, we just call the measureElement function when the height changes.

This is useful if you want to measure height in a custom way, and the element instance alone doesn't provide enough information to compute the height.

I'm curious if you would be open to PRs implementing some sort of custom measure element function. Either extending the current API to allow for user provided options (el, instance, userOptions) or allowing the user to override the definition completely.

@piecyk
Copy link
Collaborator Author

piecyk commented Dec 9, 2022

@lukesmurray hi, you can still provide measureElement like in my comment above to return item height, basic change here is that the measureElement will be called if ResizeObserver detects change.

@lukesmurray
Copy link

ahh gotcha that makes sense thank you @piecyk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic grid maximum update depth exceeded. Virtualizer does not update after adjusting row height
3 participants