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

Vertical scroll position should persist after setGroups call #172

Open
kodemi opened this issue Nov 7, 2019 · 9 comments
Open

Vertical scroll position should persist after setGroups call #172

kodemi opened this issue Nov 7, 2019 · 9 comments
Labels
bug Something isn't working

Comments

@kodemi
Copy link

kodemi commented Nov 7, 2019

Hi! I receive data for Timeline dynamically and have to call setItems and setGroups every time I get new data. But setGroups resets vertical scroll position (I set the parameter height in options). You can see it here: https://codesandbox.io/s/vis-timeline-vertical-scroll-reset-bmoc1

@yotamberk yotamberk added the bug Something isn't working label Nov 9, 2019
@yotamberk
Copy link
Member

yotamberk commented Nov 9, 2019

Try doing this:

document.getElementById("update-button").onclick = () => {
  const scrollTop = document.getElementsByClassName("vis-vertical-scroll")[0].scrollTop;
  timeline.setGroups(groups);
  setTimeout(() => {
    document.getElementsByClassName("vis-vertical-scroll")[0].scrollTop = scrollTop ;
  }, 0);
};

If this isn't good enough for what you're looking for, feel free to reopen the issue

@kodemi
Copy link
Author

kodemi commented Nov 9, 2019

Hi! I tried to set timeout and restore scrollTop, but it leads to flicker. I have updated codesandbox example to demonstrate it.

@kodemi
Copy link
Author

kodemi commented Nov 12, 2019

Should be reopened but I have no rights to do it.

@kodemi
Copy link
Author

kodemi commented Nov 17, 2019

Hi! I tried to set timeout and restore scrollTop, but it leads to flicker. I have updated codesandbox example to demonstrate it.

vis-timeline

@yotamberk yotamberk reopened this Nov 17, 2019
@kodemi
Copy link
Author

kodemi commented Dec 1, 2019

I found that the problem appeared in v6.0.0. Comparing v6.0.0 with v5.1.0 and digging through code I found traces of problem:

  1. setGroups call
  2. Core.js _redraw() call and redraw of ItemSet
  3. ItemSet redraws every group to calculate the sum of the height of the groups.
  4. Group during redraw sets it's height to 0 and ItemSet sets it's height prop and frame height style to minHeight
  5. _redraw() calls _updateScrollTop() and because this.props.center.height = 30 (minHeight) resets scrollTop to 0.

So group is failed to calculate its height in _calculateHeight because this.visibleItems = [] and this.props.label.height = 0. this.props.label.height will be updated to actual value only in next steps, after height calculation.

Quick fix is preset this.props.label in redraw() function in Group.js:

redraw(range, margin, forceRestack, returnQueue) {
    let resized = false;
    const lastIsVisible = this.isVisible;
    let height;
    const queue = [
      () => {
        forceRestack = this._didMarkerHeightChange.call(this) || forceRestack;
      },
      
      // recalculate the height of the subgroups
      this._updateSubGroupHeights.bind(this, margin),

      // calculate actual size and position
      this._calculateGroupSizeAndPosition.bind(this),

      () => {this._didResize.bind(this)(resized, this.height)}, // <----- adding this function call

      () => {
        this.isVisible = this._isGroupVisible.bind(this)(range, margin);
      },
...

And one of three:

  1. comment these lines in _updateItemsInRange:
_updateItemsInRange(orderedItems, oldVisibleItems, range) {
   const visibleItems = [];
    const visibleItemsLookup = {}; // we keep this to quickly look up if an item already exists in the list without using indexOf on visibleItems

    //if (!this.isVisible && this.groupId != ReservedGroupIds.BACKGROUND) {
    //  for (let i = 0; i < oldVisibleItems.length; i++) {
    //    var item = oldVisibleItems[i];
    //    if (item.displayed) item.hide();
    //  }
    //  return visibleItems;
    //} 

    const interval = (range.end - range.start) / 4;
    ...
  1. set group height in css:
.vis-panel.vis-left .vis-label .vis-inner {
  height: 40px;
}
  1. set timeline option groupHeightMode: 'fixed'

Demos:

  1. Combination of redraw() and _updateItemsInRange() changes: https://codesandbox.io/s/vis-timeline-jg9o6
  2. Combination of redraw() and group height style: https://codesandbox.io/s/vis-timeline-hycf1
  3. Combination of redraw() and option groupHeightMode: 'fixed': https://codesandbox.io/s/vis-timeline-ou1o0

@mckindd
Copy link

mckindd commented Jun 11, 2021

Is there any updates on a bug fix for this ticket?

I am running into the same issue in my current project. We had tried the original suggested fix to change the element a while back and that did not work or flickered depending on where it was used in React. The issue recently came back into discussion.

Any updates would be appreciated.

@TomHiller-swd
Copy link

TomHiller-swd commented Sep 9, 2022

@kodemi Thank you for this, I used the first option with patch-package to fix it.

@marcortw
Copy link
Contributor

Four years down the road, I'd also like to thank you @kodemi 😄 For me it did the trick with just the added call to _didResize. I didn't have to set any of the height specific values. But that could as well be because I don't use any height settings at all, might be why it works for me without adding one of the three other options.

marcortw added a commit to marcortw/vis-timeline that referenced this issue Jul 13, 2024
@marcortw
Copy link
Contributor

Added PR #1814

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants