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

Fix vertical scroll indicator placement #859

Merged

Conversation

ahmacleod
Copy link
Contributor

@ahmacleod ahmacleod commented Jan 28, 2021

Addresses bottom scroll indicator placement when footer is large enough to scroll. Scroll indicator drops down to bottom of table when body is fully scrolled and obscured footer rows begin to scroll into view. Docs updated to show this example.

20210129 ET Footer Scrolling V2

Note the border rendering issues with the sticky footer cells. I double-checked, and this is existing behavior. Looks to be related to #771.

@ahmacleod ahmacleod force-pushed the alex.macleod.temp/fix-vertical-scroll-indicator-placement branch from d071cb4 to 5d0904a Compare January 28, 2021 23:40
@ahmacleod ahmacleod changed the title [WIP] Fix vertical scroll indicator placement Fix vertical scroll indicator placement Jan 29, 2021
return generateRows(1);
return generateRows(100, 1, (row, key) => {
return String.fromCharCode(key.charCodeAt(0) + 7);
});
Copy link
Member

Choose a reason for hiding this comment

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

By making this change here, have we lost test coverage for cases where the footer rows are not scrollable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change only affects the example in the docs. We could include an example for each case (single footer row vs. many scrollable), but this effectively demonstrates everything the bottom indicator does.

It's fair to ask the same question of the tests, however. I'll make sure both are covered explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ahmacleod ahmacleod requested review from a team and jiayingxu January 29, 2021 22:02
Copy link
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

Very cool.

@ahmacleod ahmacleod merged commit 1e02810 into 3.0-beta Jan 29, 2021
@ahmacleod ahmacleod deleted the alex.macleod.temp/fix-vertical-scroll-indicator-placement branch January 29, 2021 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants