-
Notifications
You must be signed in to change notification settings - Fork 32
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
DataView bugfixes #2347
DataView bugfixes #2347
Conversation
Looks good to me code-wise, if you need a re-approval after changing the target branch let me know. |
Do we want to attempt to add tests to cover these fixes? Probably just the results per page one. Not sure we'd want to test styling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick skim, looks fine. I'd like to get an Odyssey design engineer to approve though.
The base branch was changed.
This latest commit fixes the broken table virtualization. It also adds an automatic scroll-to-bottom when clicking the "Load More" button, because that interaction is super weird otherwise (you can load in new rows and never realize). |
@@ -119,6 +119,7 @@ | |||
"react": "^18.2.0", | |||
"react-dom": "^18.2.0", | |||
"regenerator-runtime": "^0.14.1", | |||
"resize-observer-polyfill": "^1.5.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package was last published 6 years ago, but many people are using it, and it's MIT licensed, so we should be fine.
I think this is kind of unnecessary, but once I get Playwright, we'll have it figured out.
Two bugfixes in response to @zhengchen-okta discoveries:
loadMore