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

account for number of floating bottom rows in lastTop #300

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

spasovski
Copy link
Contributor

fixes #299

This seems to work as expected now.

@spasovski spasovski self-assigned this Nov 17, 2021
@spasovski spasovski requested a review from mofojed November 17, 2021 03:38
@spasovski spasovski added the bug Something isn't working label Nov 17, 2021
@@ -618,7 +618,7 @@ class GridMetricCalculator {
y += rowHeight;

if (y >= visibleHeight) {
return Math.min(lastTop + 1, rowCount - 1);
return Math.min(lastTop + floatingBottomRowCount, rowCount - 1);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right... so the params passed in to getLastTop include visibleHeight, which should be the height of the "visible" viewport (for lack of a better name), ie. everything that's not floating. So visually:

|-------------------------------|
| floatingTopRowHeight |
|-------------------------------|
|                                     |
|       visibleHeight          |
|                                     |
|-------------------------------|
| floatingBottomRowHeight |
|-------------------------------|

This algorithm is looking for the lastTop, which is the row that is at the top when scrolled so the last row is visible.
When we have floating bottom rows, we need to make sure the last non-floating row is visible, so we start at let lastTop = Math.max(0, rowCount - floatingBottomRowCount - 1); (last non-floating row), and iterate backward adding the height of each row until we exceed the visibleHeight, and then return the row after that (eg. lastTop + 1 in this case). Adding floatingBottomRowCount doesn't make sense - if floatingBottomRowCount is 0, then you're lastTop is going to be one row higher than it should be, and you won't be able to scroll to the bottom when there's no floatingBottomRowCount. You can observe this by checking the style guide (http://localhost:4000/styleguide)

I think this algorithm is correct here (before the change), so the issue may be with the visibleHeight that's passed in...

Copy link
Member

Choose a reason for hiding this comment

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

Yea - check where getLastTop is called from initializing lastTop:

    const lastTop = this.getLastTop(
      state,
      null,
      height - gridY - scrollBarSize
    );

That height - gridY - scrollBarSize isn't taking into account the floatingBottomHeight at all. Need to take that into account. I don't think you need to worry about the floatingTopRowHeight because the top scrolled row is scrolled underneath the floating top rows, but try playing around with MockGridModel by changing up floatingTop/BottomRowCount and testing in the style guide (would be nice to add controls straight into the style guide to modify stuff, but meh)

@spasovski spasovski merged commit 042cc9c into deephaven:main Nov 17, 2021
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

Successfully merging this pull request may close these issues.

aggregations/floating rows cover the last visible table row
2 participants