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

ScrollRowIntoView does not seen to be working with latest release #781

Closed
BuyInClub opened this issue May 20, 2023 · 9 comments
Closed

ScrollRowIntoView does not seen to be working with latest release #781

BuyInClub opened this issue May 20, 2023 · 9 comments

Comments

@BuyInClub
Copy link

If you go to the URL , https://buyinclub.github.io/Table1-Details.html?hand=20 you will notice the table always starts at the beginning of the table.
The logic is in this file - https://buyinclub.github.io/handDetails.js
The data is in this file - https://buyinclub.github.io/Table1-Details.js

I debugged into the function gotoHand and it is returning the correct row # which starts at line 301 in https://buyinclub.github.io/handDetails.js

Any help really appreciated.
thanks, Dave

@ghiscoding
Copy link
Collaborator

SlickGrid uses Virtual Scrolling to make it fast and because of that the row number keep changing when you use DataView, You need to first get the row number in the DataView by its Id before scrolling to the correct location in the virtual scroll. So you need to first find the row number in the DataView with dataView.getRowById(), something like below.

const rowNumber = dataView.getRowById(itemId);
grid.scrollRowIntoView(rowNumber, false);

The only example we have with scrollRowIntoView is Example Highlighting and it works correctly but without any DataView

@BuyInClub
Copy link
Author

Sorry for the delayed response. This did not fix the issue. This was working in the prior version, so it is a regression.

@ghiscoding
Copy link
Collaborator

The reason is because you're calling scrollRowIntoView() before the Grid is even displayed (rendered), if you add a delay with setTimeout then it works. jQuery had (sometime hacky) ways of calculating sizes even when elements aren't displayed but I don't think we need to support this in native code.

There 2 ways of fixing your issues,

  1. add a delay
setTimeout(() => grid.scrollRowIntoView(gotoHand()-1), 0);
  1. wait for grid to be rendered
grid.onRendered.subscribe(() => {
    grid.scrollRowIntoView(gotoHand()-1);
});

The option 2 is probably better and I tried them both on your site and they both work.

In summary, I'm very surprised that the previous code was working (allowing) to call scrollRowIntoView at grid creation. You should always wait for the DOM to be ready before doing such thing as a scroll (or anything else that requires the grid to exist in the DOM before doing certain action)

So I'm closing this as "work as intended" and you need to choose 1 of the 2 proposed code change.

image

@BuyInClub
Copy link
Author

BuyInClub commented May 29, 2023

I tried both options, the second, wait for the grid to be rendered, does not allow the grid to be scrolled past the selected row,
That is not an option it is not a workable solution for me.

The first, the setTimout, works and allows scrolling; however the row is at the bottom of the grid. I would like it at the top. (This is the case for both solutions). If I know how many rows are in the viewable grid I can add that #. Is there a way of determining this?

Thank in advance for your help.
Dave
P.S. You also maybe surprised that in the earlier version, the scrollRowIntoView, rendered the row at the top of the grid.

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 29, 2023

As far as I can see in the PR that brought the new major release (PR #777), the only thing that changed in the scrollRowIntoView is how to get the viewport scroll height is pulled, the rest of the calculation is exactly the same as before. So I can only assume that the previous version was also scrolling to the same position (which is to make the cell available at the bottom of the grid).

This is an open source project, if you want to make this better then you can always contribute to the project. Perhaps there could be another optional argument to the function to scroll in the middle of the screen by calculating the visible viewport divided by 2... I would suggest you to contribute to the project, since I don't have time to work on this since I am already working on the next major version instead.

If you think it was different in the previous version, then you should troubleshoot it yourself by adding a breakpoint into slick.grid.js to the viewport height variable and compare both versions if they return the same height because like I said jQuery has different ways of calculating height for elements that aren't visible. It's easier to troubleshoot when you own the code and have it locally (which we don't have), you should also troubleshoot against unminified code (.js instead of .min.js)

image


I also checked on the original SlickGrid (from the original author MLeibman) and the scrollRowIntoView is also scrolling to the bottom of the grid as you can see in the animated gif... so the behavior seems to be exactly the same as it always was.

The example shown below is available at these 2 locations

msedge_jXAESUvFWF

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 31, 2023

@BuyInClub
So I took another look and as far as I can see it works exactly like it should, I didn't know that but there's actually 2 methods available to scroll

  • scrollRowIntoView(x) will scroll and show the row x at the bottom of the grid
  • scrollRowToTop(x) will scroll and show the row x at the top of the grid

So you should be using scrollRowToTop() instead of scrollRowIntoView(), see animated gif below

As for the reason you need to add a delay, I think it might be because the data is not fully loaded before you scroll, you have a lot of data that are loaded async and because of that the scroll height is not fully calculated so you can't scroll yet for that reason.

The setTimeout is a patch, but a better fix is to add your scroll inside the DataView onRowCountChanged event since you know that at that time the data is fully loaded so the scroll height is correctly calculated, so this code should work

let alreadyScrolled = false;

dataView.onRowCountChanged.subscribe((e, args) => {
  grid.updateRowCount();
  grid.render();

  // scroll only after loading the full dataset `> 0` but only scroll once
  if (!alreadyScrolled && args.itemCount > 0) {
    grid.scrollRowToTop(255);
    alreadyScrolled = true;
  }
});

So I think that concludes the troubleshooting and the label is correct which is "work as intended" and the reason it was working before with jQuery was maybe by luck

Code_shiMI8oGH3

@BuyInClub
Copy link
Author

Awesome, this line did it -
setTimeout(() => grid.scrollRowToTop(rowNum), 0);

I was working on my own method which was doing something like this -
var x = Math.round(resizer.getLastResizeDimensions().height / 25);
setTimeout(() => grid.scrollRowIntoView(rowNum + x-1), 0);

scrollRowToTop is much more exact!

I will look into onRowCountChanged. If this gets triggered when the number of rows changes, then this may not work for me. The # of rows never change in the table.

Many, many thanks.
Dave
P.S I am a novice at JavaScript. I find tracing into JavaScript code confusing. I am use to strongly typed, compiled languages. The DOM and other libraries make it challenging for me. This is not a knock, it just means it is difficult for me to contribute.
P.S.S. The version I was using was not the original from mlieberman, it was the 3.x release from 6pac.github.io that scrollRowIntoView worked for me. As you mentioned, perhaps luck.
I was going to go back to the 3.x version, but not going to waste the time now.

3.x libraries I was using are below -

<script src=https://6pac.github.io/SlickGrid/lib/firebugx.js></script> <script src=https://6pac.github.io/SlickGrid/lib/jquery-3.1.0.js></script> <script src=https://cdn.jsdelivr.net/npm/flatpickr></script> <script src=https://cdn.jsdelivr.net/npm/sortablejs/Sortable.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/slick.core.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/slick.interactions.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/slick.formatters.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/slick.editors.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/plugins/slick.rowselectionmodel.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/slick.grid.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/slick.dataview.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/controls/slick.pager.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/controls/slick.columnpicker.min.js></script> <script src=https://6pac.github.io/SlickGrid/dist/plugins/slick.resizer.min.js></script>

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 31, 2023

the onRowCountChanged gets triggered whenever the row count changes in the grid, that is also triggered when you filter data in the grid and it gets called at least once when you load the data with setItems... actually there's also the onSetItemsCalled that could probably be used and might be better for your use case

@6pac
Copy link
Owner

6pac commented May 31, 2023

I beleive we have three DataView row data events now:

onRowCountChanged
onRowsChanged
onRowsOrCountChanged

These should cover all the scenarios a developer would face. We added onRowsOrCountChanged because previously it was a bit messy if you wanted an event to happen in both onRowCountChanged and onRowsChanged - if both changed as a result of a single action it would end up being called twice.

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

No branches or pull requests

3 participants