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

[v4] [table] fix: prevent scrolling to ghost rows #5115

Merged
merged 11 commits into from Feb 10, 2022

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Feb 3, 2022

Fixes #5027

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Add Table2 vertical scroll logic to detect when scrolling has reached the bottom of the rendered cells, so it's not necessary to allow scrolling further down to the ghost cells.

Edit: that approach was quite buggy, so I figured out a better one. We don't need to render ghost rows or columns at all, when there is vertical or horizontal overflow, respectively.

Reviewers should focus on:

This should fix the linked bug.

Screenshot

with ghost cells enabled in the table dev app:

2022-02-10 13 54 56

2022-02-10 13 55 12

@blueprint-bot
Copy link

[table] WIP fix ghost cells bug

Previews: documentation | landing | table | modern colors demo

@adidahiya adidahiya marked this pull request as ready for review February 4, 2022 18:35
@adidahiya adidahiya changed the title [v4] [table] WIP fix ghost cells bug [v4] [table] fix: prevent scrolling to ghost rows Feb 4, 2022
@blueprint-bot
Copy link

[table] fix ghost cells scrolling

Previews: documentation | landing | table | modern colors demo

@blueprint-bot
Copy link

Fix linting, formatting

Previews: documentation | landing | table | modern colors demo

@adidahiya
Copy link
Contributor Author

if you scroll too fast, you get blank sections of the table on the bottom / right where the viewport does not update:

image

@blueprint-bot
Copy link

WIP another strategy

Previews: documentation | landing | table | modern colors demo

@blueprint-bot
Copy link

Remove dead code

Previews: documentation | landing | table | modern colors demo

packages/table/src/table2.tsx Outdated Show resolved Hide resolved
});

function runTestToEnsureScrollingIsEnabled(enableGhostCells: boolean) {
it(`isn't disabled when there is half a row left to scroll to and enableGhostCells is set to ${enableGhostCells}`, () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that these two tests were ported over from 7460d10

packages/table/test/table2Tests.tsx Outdated Show resolved Hide resolved
@blueprint-bot
Copy link

fix lint, formatting

Previews: documentation | landing | table | modern colors demo

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.

None yet

2 participants